#463: Add ability for timeout to print full thread stack dump #768
Conversation
junit-team#463: Add ability for timeout to print full thread stack dump - Naive attempt that (a) optionally adds information from ThreadMXBean#findMonitorDeadlockedThreads and (b) always adds full thread dump to exception message
| } | ||
|
|
||
| public FailOnTimeout(Statement originalStatement, long timeout, TimeUnit unit, boolean lookForStuckThread) { | ||
| public FailOnTimeout(Statement originalStatement, long timeout, TimeUnit unit, boolean lookForStuckThread, boolean fullThreadStackDump) { |
There was a problem hiding this comment.
IMHO this is far too many parameters. Could we create a Builder for FailOnTimeout and get rid of all of these constructors? See Effective Java, Second Edition, Item 2.
I personally like the style where there is a private constructor that takes in an instance of the Builder.
|
@kcooney and @kalendamoise: Secondly I must apologize in advance for most likely spending hardly any time on this in the next 1 to 2 weeks; but certainly afterwards I will. Thirdly, please note that this was just a quick draft trying to (a) bring up the topic and (b) show with minimalistic changes (mimicing the existing style of the the look-for-stuck-thread) what could be useful: but in my humble opinion this only provides the big benefit in combination with a Java 6++ VM by being able to provide ThreadMXBean lock, monitor and synchronizers information. Which would not be possible like this, but only by either (1) an external handler as outlined in #770, or (2) by converting JUnit to a multi-project build (allowing the Java 6++ full-thread-stack-dump in a new mini project; but nonetheless creating a single JAR artefact). => So what do you think? Should I/we persue this limited attempt nonetheless? |
|
@reinholdfuereder Are you saying that this change depends on #770 or are you saying that this change wouldn't be all that useful without those changes? Either way, I think @dsaff would have to comment on those issues |
|
@kcooney Well. I think there may be situations where this (limited) approach can help; but IMHO there may be also rare other situations (maybe rather complex third-party code and/or rather bad coding/design/architecture?), where this extended information might be needed or at least might accelerate the failure analysis significantly.
|
|
@reinholdfuereder Can we close this off in favor of #797 ? |
|
@kcooney I personally think that it would be an appreciated JUnit feature to provide an out-of-the-box approach to get/collect context when a test runs into a timeout. E.g. if the new stuck thread detection does not help and/or there is just a sporadically occurring problem (due to a synchronization bug or whatever). |
|
@reinholdfuereder I think i would prefer to see if we can make extension points so that, for example, developers can make their own version of the timeout rule with this behavior. I can imagine some environments where there are other types of analysis you might do if there is a stuck thread, or other formats to print out the results. Is having the custom exception thrown on timeout not help here? You could write a rule that wraps the timeout rule and do extra behavior when the timeout exception is thrown |
|
@kcooney this sounds very good to me maybe I (one last time) try to explain some use cases why I nonetheless hope for some out-of-the-box support by JUnit: (a) let's say some devs were writing/commiting tests as unit tests that were in fact rather integration tests; and so the global timeout approach of #772 with an increasingly lower timeout should lead to the failing of these integration tests, so that there is a force to categorize or refactor them; however, for unit tests there is no dedicated rule setup via e.g. a dedicated JUnit runner, but nonetheless one may be interested into why the test actually run into the timeout; (b) and for integration tests it may be quite similar: no dedicated rule setup due to using the default JUnit runner and in case of a timeout (e.g. due to a multithreading bug), there is also no failure context at all; and of course you then cannot re-produce this problem on your machine... |
|
@reinholdfuereder the problem with out-of-the-box support in JUnit is that it's likely we won't get this right the first time. Having extension points allows developers to solve it their own way, and allows us to see what they've done when we go about adding the functionality to the JUnit core. I would like to keep the discussion on #772 separate. I do think you should be able to roll you own version of this pull via existing extension points. If there's some reason you can't, I'd very interested in hearing what you've tried. |
|
@kcooney I think only now (and with your comment on #772 (comment)) I really got your point, and I must admit that this sounds very convincing at a first thought. Thanks for your patience and this idea. Unfortunately I currently do not know when I will have time for such an attempt, but this is definitely worth it. |
|
(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 |
Although originally this started based on #727: Fail on timeout displays stack of stuck thread …
This is a first naive attempt that (a) optionally adds information from ThreadMXBean#findMonitorDeadlockedThreads and (b) always adds full thread dump to exception message
@dsaff and #727: 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 very ugly thoughts, when I want to have this extended thread dump?
** => actually this is roughly implemented in Full thread stack dump on timeout with custom handler #770