#727 Fail on timeout displays stack of stuck thread

[adam-beneschan]

New class: ExceptionWithThread.java (subclass of Exception that keeps information about some other relevant thread). Changed: FailOnTimeout.java (starts test in a new ThreadGroup; on timeout, looks at running threads in ThreadGroup to see which one it thinks got stuck, creates an ExceptionWithThread if it finds one); Failure.java (displays stack for the "relevant thread" if it handles an ExceptionWithThread)

[dsaff]

Can you reformat to match the formatting of the surrounding classes?

[dsaff]

Thank you very much for tackling this. This is a good direction.

[adam-beneschan]

You're welcome. I'll take a look at your suggestions soon.

[adam-beneschan]

@dsaff I've made the changes you suggested. Since the new version has just one changed file (FailOnTimeout.java) instead of two changed files and one new one, I created a branch in my repo, issue727-2. (Changes to Failure.java have all been backed out.) Do I need to make another pull request, or is this notification sufficient, or is there anything else I need to do? I'm still trying to learn the mechanics of GitHub; apologies if I did something wrong. The "master" branch in my repo is still the changes from three days ago, so it isn't what you want. Also let me know if there's an indentation problem; things look fine in Eclipse but I was having some problems with tabs. I noticed that with MultipleFailureException, a failure that also displays the stack of a "stuck" thread will now be counted as 2 failures. Maybe that's best. If not, and if it ought to be counted as one failure, I'm sure something could be done (possibly a boolean field in MultipleFailureException to indicate the multiple exceptions are "really just different aspects of one failure", or maybe there's a better solution).

[dsaff]

@adam-beneschan,

You'll need to create a new commit on the same branch, and then push it to your repo. No problem, GitHub takes turning your head sideways for a while until it starts making sense.

I think two failures is fine; when I can take a look at the new commit, I can comment more.

[adam-beneschan]

OK, I hope I did it right this time. Thanks for your help.

-----Original Message-----
From: David Saff notifications@github.com
To: junit-team/junit junit@noreply.github.com
Cc: adam-beneschan adambeneschan@aol.com
Sent: Mon, Sep 30, 2013 2:39 pm
Subject: Re: [junit] #727 Fail on timeout displays stack of stuck thread (#742)

@adam-beneschan,
You'll need to create a new commit on the same branch, and then push it to your repo. No problem, GitHub takes turning your head sideways for a while until it starts making sense.
I think two failures is fine; when I can take a look at the new commit, I can comment more.
—
Reply to this email directly or view it on GitHub.

[dsaff]

Still odd indentation here.

[dsaff]

I like this version. Some structure and formatting questions. You can take a look on GitHub to see if spaces and tabs are matching up: we generally indent just with spaces.

It should be possible to write a self-test for this feature, don't you think?

[adam-beneschan]

@dsaff OK, I've made the improvements you suggested. (I got Eclipse to stop generating tab characters.) Also, I added two test cases to org.junit.tests.running.methods.TimeoutTest (one multithreaded case where a "stuck thread" should show up, and another multithreaded case where the main thread is the stuck one so the result shouldn't look any different). I couldn't figure out what org.junit.tests.internal.runners.statements.FailOnTimeoutTest was; it doesn't seem to be referenced by anything else. Is that something that needs modification also?

[dsaff]

If we move this below the if (stackTrace != null) check below, then we can just use return statements, rather than the resultException local

[dsaff]

Quite nice. A sprinkling of style requests throughout.

Could this create problems for anyone? I'm explicitly wondering what the computational costs of the thread enumeration are, and whether they're high enough that someone might want a switch to turn it off?

[dsaff]

Thanks! Can you add notes at https://github.com/junit-team/junit/wiki/4.12-release-notes? Thanks.

[dsaff]

Oh no! Arrays.copyOf is Java 1.6, and we're still trying to maintain compatibility down to 1.5. Can you make a follow-on pull request with a fix? Thanks.

[adam-beneschan]

@dsaff Done (for #742). I can do #759 after it's merged (removed use of Arrays.copyOf).

Thanks for all your help with the GitHub mechanics. I'm hoping to be able to make more contributions to JUnit in the future. Thanks, Adam

-----Original Message-----
From: David Saff notifications@github.com
To: junit-team/junit junit@noreply.github.com
Cc: adam-beneschan adambeneschan@aol.com
Sent: Thu, Oct 31, 2013 12:12 pm
Subject: Re: [junit] #727 Fail on timeout displays stack of stuck thread (#742)

Thanks! Can you add notes at https://github.com/junit-team/junit/wiki/4.12-release-notes? Thanks.
—
Reply to this email directly or view it on GitHub.

[kcooney]

@dsaff Java 5 reached end of life on October, 2009, and Java 6 reached EOL in February, 2013:
http://www.oracle.com/technetwork/java/eol-135779.html
http://slashdot.org/story/13/06/20/1819245/java-6-eold-by-oracle

I think we should ask on the community if there is any reason why JUnit should continue to support Java 5.

[kcooney]

Hello, @adam-beneschan

When I was looking at issue #388 I noticed that both FailOnTimeout and the Timeout rule where changed in ways that might not be thread-safe.

The recent changes to FailOnTimeout may have made it not thread-safe since fThreadGroup is neither final nor volatile. That being said, I'm not sure why a Statement would be used by multiple threads. An instance of FailOnTimeout is created in BlockJUnit4ClassRunner when it creates the statement, and the statement should only be called by one thread.

It couldn't hurt to make fThreadGroup volatile. Alternatively, you could remove fThreadGroup and pass the ThreadGroup into getResult(). Or, if threading the ThreadGroup through two methods is too yucky, create a nested class that extends FutureTask.

The changes to the Timeout rule may be less safe, since in theory you could use this rule as a class rule, which means that the Timeout instance is static and may be accessed from multiple threads.

Perhaps Timeout.lookForStuckThread() should be renamed to Timeout.lookingForStuckThread() and return a new instance of Timeout so that fLookForStuckThread can be final? Alternatively, make fLookForStuckThread volatile and make Timeout.lookingForStuckThread() synchronous. Personally, I would have made a builder:

@Rule public final Timeout timeout = Timeout.builder()
    .lookingForStuckThread()
    .build(100, TimeUnit.SECONDS);

[reinholdfuereder]

@kcooney there is still a lot of old code out there needs to run with Java 1.5 and thus should be testable with Java 1.5; and now it gets really extreme: in the company I am working for we are developing a product called dynaTrace that must be still Java 1.4.2 compatible, because clients are using it inside their Java 1.4.2 applications...

[reinholdfuereder]

@adam-beneschan and @dsaff et al. I really like this new feature's direction! A few years ago we were implementing kind of related features in admittedly quite proprietary and quite ugly manner (due to being based on JUnit 4.4: no Rules!), and below I try to list some thoughts and extension ideas:

• If a test is "just" running into a timeout, I very often would like to have a complete stack trace of all threads (instead of "just" the single stuck thread). Of course this is getting much more important the bigger the JUnit technology based test type is (e.g. integration tests). Often times the threads are playing together very closely, and/or maybe something is not really stuck, but just unexpected slow...
• In addition to a stack trace in really big test types based on JUnit, i.e. system tests, I then not only want a stack trace of the current test process, but also a kind of callback hook or extension point to allow executing custom/specific/proprietary actions to enrich/extend the test failure context, e.g. by triggering a dump of other involved system that are running out-of-process...
• For big CI environments with let's say thousands of tests I don't want to specify a timeout on each single test (class), and I also do not want to force devs to use a specific proprietary test runner for their test (classes), but would prefer a configuration option for a tolerant global default timeout via a system property. This timeout should be used, unless a test case or test class specifies a more specific timeout.

I would be very happy, if the implementation of this issue and/or supplementing issues could take these thoughts/ideas into account.

Thanks, Reinhold

[dsaff]

@reinholdfuereder,

I can't address all of your issues at one time, but for just the first one, one thing that might help is if Timeout#createTimeoutException were protected, so that you or anyone could create your own subclass of Timeout that would gather whatever data you wanted at the time of a timeout. If you wanted to put together a pull request for that, I'd be on-board.

[reinholdfuereder]

@dsaff alright, that sounds like a plan: I am going to have a look at this in the course of either this or next week

[reinholdfuereder]

Oh wow! #463 looks very familiar to my comments...

[reinholdfuereder]

@dsaff: Since the Timeout#createTimeoutException is now (?) in FailOnTimeout#createTimeoutException (statement) it would require to (1) subclass FailOnTimeout to override FailOnTimeout#createTimeoutException and (2) subclass Timeout to override Timeout#apply to create and return an instance of the new FailOnTimeout sublcass from before. I had hoped for more convenience...

Besides that I also stumbled over the fact that quite some interesting thread dump information (locks, monitors, synchronizers) can only be retrieved from ThreadMXBean from Java >= 1.6.

Thus, what do you think of the following naive and admittedly ugly thoughts, when I want to have this extended thread dump?

• Is any poor mans plugin/extension attempt via an non-elegant reflection based class creation (e.g. there is a system property with the full qualified name of the dumping class) thinkable?
• Another ugly idea would be making JUnit a multi-project with an optional 1.6 compatible part, where this extended thread dump class can be placed. Of course this then could be packaged in whatever ugly style in a single JAR and again via system property + reflection plugged in... But this gets really dirty...

(See also my first naive pull request for #463 for discussion)

[dsaff]

@reinholdfuereder,

Have you tried doing what you laid out in paragraph 1? Was it as difficult as you feared?

[reinholdfuereder]

@dsaff, do you refer to the paragraph ending with "I had hoped for more convenience...", or to first item in the thoughts?

In order to provide some food for discussion I have committed some rudimentary approaches into these pull requests: #768, #770 and #772

[dsaff]

I was thinking about "(1) subclass FailOnTimeout to override FailOnTimeout#createTimeoutException and (2) subclass Timeout to override Timeout#apply to create and return an instance of the new FailOnTimeout sublcass from before"

This doesn't feel overly onerous to me. I was curious if you'd tried it and found it more difficult than I suspected.

[reinholdfuereder]

@dsaff: Okay, sorry for my confustion => now I tried...

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)
(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 to a branch (but not made a pull request yet): https://github.com/reinholdfuereder/junit/compare/customtimeout-and-customfailontimeout

(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).