Explain why the thread group is destroyed #1690
Conversation
stefanbirkner
force-pushed
the
thread-group-not-leaked-2
branch
2 times, most recently
from
b184303
to
394f935
Compare
stefanbirkner
force-pushed
the
thread-group-not-leaked-2
branch
from
394f935
to
814eaa7
Compare
stefanbirkner
force-pushed
the
thread-group-not-leaked-2
branch
from
814eaa7
to
e5711e2
Compare
This information is currently only available in GitHub. It should be close to the code so that the reader can understand why we destroy the thread group.
stefanbirkner
force-pushed
the
thread-group-not-leaked-2
branch
from
e5711e2
to
f035b32
Compare
| @@ -121,6 +121,12 @@ public void evaluate() throws Throwable { | |||
| CallableStatement callable = new CallableStatement(); | |||
| FutureTask<Throwable> task = new FutureTask<Throwable>(callable); | |||
| ThreadGroup threadGroup = new ThreadGroup("FailOnTimeoutGroup"); | |||
| // The thread group must be a daemon thread group so that it is | |||
There was a problem hiding this comment.
nit: blank line above this line for readability.
| @@ -121,6 +121,12 @@ public void evaluate() throws Throwable { | |||
| CallableStatement callable = new CallableStatement(); | |||
| FutureTask<Throwable> task = new FutureTask<Throwable>(callable); | |||
| ThreadGroup threadGroup = new ThreadGroup("FailOnTimeoutGroup"); | |||
| // The thread group must be a daemon thread group so that it is | |||
| // destroyed when the time-limiting thread finishes. Only when the | |||
| // thread group is destroyed its parent thread group can be destroyed, | |||
There was a problem hiding this comment.
I'm a bit puzzled by this explanation.
The parent thread group isn't destroyed by any of the code here. The parent thread group will likely live for the life of the thread that ran this Statement, which is likely the life of the test run.
Looking at the original pull, it sounds like ThreadGroupContext is likely java.beans.ThreadGroupContext, and Spring uses that to store data, using the identity hash code of the current ThreadGroup as a key. So I suggest:
"By ensuring the ThreadGroup is destroyed, any data associated with the ThreadGroup (ex: via java.beans.ThreadGroupContext) is destroyed."
Presumably, any tests not using a timeout would have data stored using the ThreadGroup from the main JUnit thread as the key for the context, and that would stay around until all tests complete. Perhaps that isn't a problem because the data doesn't keep growing as more tests are run.
There was a problem hiding this comment.
Thanks for digging deeper into it. #1691 makes this PR obsolete, isn't it?
There was a problem hiding this comment.
@stefanbirkner I tried to capture the gist of the comments that you added here in my pull. Please let me know if you want me to say more.
Thanks for the reminder to document why the code does what it does
|
I'm closing this PR because #1691 is changing the code and adds comments with a better explanation. |
This information is currently only available in GitHub. It should be
close to the code so that the reader can understand why we destroy the
thread group.