Completely custom timeout approach as suggested by dsaff in #727

[reinholdfuereder]

As #727 is already merged and I therefore think kind of resolved, I created this pull request to avoid forgetting this approach as suggested by dsaff near the end of the conversation to #727. Please note that I also think

The following lines/thoughts are an edited copy of #742 (comment):

---

I think that in addition to these 2 steps, it is also necessary:
(3) to allow access to all fields of Timeout (i.e. fTimeout, fTimeUnit and fLookForStuckThread) either via changing its visibility from private to e.g. protected, or via a getter (with e.g. protected visibility) => for now with changed visibility for the sake of less change, what do you prefer?
(4) since FailOnTimeout#createTimeoutException contains some bits of logic that should be re-used instead of being duplicated => it should be called too in the custom overridden method first => then additional nesting of the returned Exception (it may already be a MultipleFailureException) might be needed (to provide either the additional failure context in the test failure, or to provide a hint in the test failure that our custom FailOnTimeout may have provided additional failure context e.g. in the log output or whatever)

I have made the changes and added tests.

(Side note: in case CustomTimeoutTest.infiniteLoopwithLookForStuckThread() is executed before CustomTimeoutTest.infiniteLoop() the full stacktrace on timeout of the second test shows the surviving threads from the first test, which is using InfiniteLoopMultithreaded from the original TimeoutTest => presumably that is not on purpose...)

However, please note that (as I tried to convey in the older pull requests #770 and #772) I am still hoping for "more" allowing to cover these outlined real world use cases (in particular without the risk of missing certain tests).

---

[stefanbirkner]

The CustomTimeoutTest has to be added to the test suite AllTests.

[reinholdfuereder]

@stefanbirkner Thanks for the hint/suggestion; hopefully the commit above addresses this accordingly.

[stefanbirkner]

@reinholdfuereder It does. Thanks.

[kcooney]

I personally don't like the idea of having protected non-final fields.

Why is this non-final to begin with? Could lookForStuckThread() return a copy of the original?

[reinholdfuereder]

ad "protected non-final fields": I don't like it either, including this inconsistency, but this was part of the original forked code already. And in this concern I also wrote in the conversation/description of this issue:

(3) to allow access to all fields of Timeout (i.e. fTimeout, fTimeUnit and fLookForStuckThread) either via changing its visibility from private to e.g. protected, or via a getter (with e.g. protected visibility) => for now with changed visibility for the sake of less change, what do you prefer?

So maybe the original code should be refactored first?

[kcooney]

I'm not sure what type of refactoring you are thinking of.

I think we should do one of the following

a. make fLookForStuck thread final, and have lookForStuckThread() return a copy
b. make all the fields private, and have protected final accessor methods

[reinholdfuereder]

While I started responding like this:

for variant a. another c'tor (taking 3 args) would be then needed as well (the third arg being assigned to this fLookForStuck thread final...

I somehow based on intuition (?) looked in the current junit head and surprisingly, there the code had been changed since my branching already really as you suggested: there is in fact a private final boolean fLookForStuckThread and well yet another c'tor taking just 2 args protected Timeout(Timeout t, boolean lookForStuckThread)...

So what about (1) merging in the changes from head to my branch, and (2) making (cf. head) or keeping (cf. my branch) these fields protected?

[kcooney]

@reinholdfuereder Instead of having the fields be protected, how about we add protected final getter methods to get the values? That may give us a bit of flexibility in the future.

[reinholdfuereder]

@kcooney I just did that

[kcooney]

@reinholdfuereder this still looks like more test code than is really necessary to test the changes. You simply need to verify that 1) the subclass version of the method is called, and 2) the return value is used. Could your tests just test that?

[reinholdfuereder]

@kcooney while I was thinking about how to accomplish that (and thinking whether or not I need to add a mocking library for that purpose, and if that were allowed), I finally found out why I am kind of hesitating or thinking that may not be fully sufficient or so => Can you please have a look at the recent changes which actually address my previous comment:

(Please note I am not fully happy about CustomFailOnTimeout.createTimeoutException() yet: while I have full flexibility in my custom timeout exception creation, it feels a bit awkward.)

[kcooney]

@reinholdfuereder I'm confused. It looks like you changed the non-test code a day ago.

Can we take a big step back and ask what problem you are trying to solve? It looked like you were originally trying to add some simple extension points, but now I'm not sure what you are proposing.

(and to answer your question, we haven't yet found the need to use a mocking framework for testing JUnit itself)

Also, I can't find the comment from dsaff in 727 about the "custom timeout approach". Did you mean #742 (comment) ? If not, could you add a link to that comment please? Sometimes github hides comments in older diffs. Thanks.

[reinholdfuereder]

@kcooney
(1) Ad comment: yes, sorry, the comment you found was actually the comment (and also the discussion/conversation below) that led to this pull request. sorry for somehow mixing that up in the pull request's title.

(2) Ad confusion: yes, indeed, I changed the production code. Because it helps a lot as you can see here (as the old createTimeoutException method was doing too much, I think):
(a) here the old code needed for custom timeouts (just not clean and a bit duplicate with respect to production code):

@Override
protected Exception createTimeoutException(Thread thread) {
    ArrayList<Throwable> exceptions = new ArrayList<Throwable>();

    Exception handlerException = handler.handleTimeout(thread);

    Exception defaultException = super.createTimeoutException(thread);
    if (defaultException instanceof MultipleFailureException) {
        MultipleFailureException defaultMfe = (MultipleFailureException) defaultException;
        exceptions.addAll(defaultMfe.getFailures());
    } else {
        exceptions.add(defaultException);
    }

    exceptions.add(handlerException);

    return new MultipleFailureException(exceptions);
}

(b) here the new one (much better, I think):

@Override
protected List<Throwable> createAdditionalTimeoutExceptions(
        Thread thread) {
    List<Throwable> exceptions = super.createAdditionalTimeoutExceptions(thread);
    Throwable handleTimeout = handler.handleTimeout(thread);
    exceptions.add(handleTimeout);
    return exceptions;
}

(3) Ad step back: I want to finish this pull request here to "allow-users-to-do-everything-with-custom-timeouts-by-themselves" as suggested by @dsaff . And in addition I would later on (in one of my other pull requests) really try the approach suggested by you with extension points based on Java 6 ServiceLoader

[kcooney]

@reinholdfuereder while I can see how these changes make your custom subclass cleaner, I would personally rather keep things simple, and not change the current functionality. I think I would personally prefer the simpler approach of just making the existing methods extendable.

@dsaff what do you think?

[reinholdfuereder]

@kcooney I am sorry, but this time I do not agree with you.

(1) Because the original simpler approach (FailOnTimeout.createTimeoutException()) is IMHO already a bit unclean (cf. SOLID etc), because it does too much and therefore looks already clumsy:

private Exception createTimeoutException(Thread thread) {
    StackTraceElement[] stackTrace = thread.getStackTrace();
    final Thread stuckThread = fLookForStuckThread ? getStuckThread(thread) : null;
    Exception currThreadException = new TestTimedOutException(fTimeout, fTimeUnit);
    if (stackTrace != null) {
        currThreadException.setStackTrace(stackTrace);
        thread.interrupt();
    }
    if (stuckThread != null) {
        Exception stuckThreadException = 
            new Exception ("Appears to be stuck in thread " +
                           stuckThread.getName());
        stuckThreadException.setStackTrace(getStackTrace(stuckThread));
        return new MultipleFailureException    
            (Arrays.<Throwable>asList(currThreadException, stuckThreadException));
    } else {
        return currThreadException;
    }
}

(2) and the old extending mechanism is IMHO unclean due to the need for duplications and/or imitating behaviour in (each) extension (cf. the code usage before vs. after as shown in my previous comment; and cf. the old usage above with the original FailOnTimeout.createTimeoutException())

[kcooney]

It sounds like we are talking past one another. @dsaff made the original suggestion to make FailOnTimeout more extensible, so lets wait for him to comment.

[dsaff]

Unfortunately, I can't remember everything I say some days, and I don't seem to see the comment that I should clarify at #727. Does someone have a pointer?

[reinholdfuereder]

@dsaff yes, sorry, I mixed something up with the issues; please see the comments starting from 6 above your comment (i.e. the one from @kcooney with "Can we take a big step back"). And of course on the very top of this conversation (pull request) there is some citation of your initial comment explaining the root of this pull request.

@kcooney in response to your comment "made the original suggestion to make FailOnTimeout more extensible" I dare to respond: right, that is what I did; thinking also carefully about the users of this extension/API

[dsaff]

This can be simplified to MultipleFailureException.assertEmpty, for what it's worth.

[reinholdfuereder]

Maybe I miss the point: but can you have a second look at that?

My code change above leads to the same behaviour as before my change: which was to collect and return the timeout exception(s): either the single exception or the multiplelfailureexception in case of more than one.

And therefore I don't know where you would like to use MultipleFailureException.assertEmpty

[dsaff]

So #742 (comment) appears to be the comment you're thinking about, which could be accomplished by making FailOnTimeout#createTimeoutException protected and making protected accessors for three of the fields. It seems that that small change would be less controversial; would it help to issue that pull request first, and then do a second pass where you present your new idea?

The second pull request would be accurately described as "Here's something I thought would help", rather than "Here's some changes @dsaff wants".

[reinholdfuereder]

I think I get your point of me drifting of with respect to the initial pull request title.

Sorry, but I was just trying to accomplish improvements instead: I think the initial approach was just not enough, as the API for usage and extension actually led to the need of duplicating/copying logic from the production code in the extension code as proven above => I personally think it is obvious, that the latest approach is superior over the initial one; of course there will always be room for further improvements...

And thus I don't want to commit the initial approach at all.

=> What about changing the title of this pull request to something like "Allow custom timeout exceptions" or "Make timeout exception creation extendible" or so? (If that were possible. Is it?)

[kcooney]

@reinholdfuereder while I think I understand your concern, it looks like the duplication would be small, and only in your subclass. It's possible other subclasses wouldn't have this duplication.

[reinholdfuereder]

Okay, I have thus make FailOnTimeout.createTimeoutException() overridable again.

However, I am still by no means convinced by your reluctance of making the API easier to extend: because as a developer, I am very happy about additional context in case of a test failure (due to timeouts), and I cannot think of a situation where one would deem that the default JUnit timeout failure context is so worthless to be intentionally removed/overridden.

[kcooney]

(doing a pass at closing old pull requests)

I'm closing this and the related thread. I think we generally agree that we need to take a step back before making changes to see what the goals are, and if there is a simpler way to accomplish them