Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Threads leaking from ForkJoinPool.commonPool (randomization in RandomCodec) #14066

Closed
dweiss opened this issue Dec 14, 2024 · 7 comments
Closed
Labels
Milestone

Comments

@dweiss
Copy link
Contributor

dweiss commented Dec 14, 2024

Description

#14049 added randomization of codec params but as part of that it may spin up new threads when ForkJoinPool.commonPool is started. We have no control over these threads but they do cause test failures as in here:

image

I think we can safely assume common pool's threads surviving the test's scope are not interesting for us to detect and any leftover threads/ runnables will just eventually complete and die (?). I feel tempted to ignore those threads by adding them to QuickPatchThreadsFilter. If we want to ensure nothing survives the test's scope, we could add a rule to LuceneTestCase that would wait (after the suite completes) until the active count of workers in the common pool is zero.

Version and environment details

No response

@dweiss dweiss added this to the 10.1.0 milestone Dec 14, 2024
@dweiss
Copy link
Contributor Author

dweiss commented Dec 14, 2024

Hmm... Interesting that I can't reproduce this locally. Also, ForkJoinWorkerThread should be demon threads, which are ignored by the default set of filters... Headscratch.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 14, 2024

This only happens with coverage tests. There is a difference in how these are started - they run without the security manager. Turns out ForkJoinPool's default factory has different settings for running with and without the security manager, resulting in system threads being either part of the test group or not. This is the direct reason why these tests pass in normal runs (most of those use the security manager) and fail with coverage runs (no security manager).

@rmuir
Copy link
Member

rmuir commented Dec 14, 2024

thanks for debugging this: good find, since security manager won't be around forever...

@dweiss
Copy link
Contributor Author

dweiss commented Dec 14, 2024

Thanks. If I read David Delabassee's e-mail correctly, JDK 24 effectively removes it internally.

I've committed a quick patch to QuickPatchThreadsFilter to ignore those daemon threads. Sorry I didn't push it as a PR for review - seemed like a trivial change with no clear alternatives. If you'd like to run through the full cycle, I can revert and push a PR.

@rmuir
Copy link
Member

rmuir commented Dec 14, 2024

no need for a hassle, thanks for fixing it. I will take care of the tidy.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 14, 2024

Damn. I'll do it, no problem. Sorry for the noise.

@dweiss
Copy link
Contributor Author

dweiss commented Dec 14, 2024

Oh, you're fast. Thank you.

@dweiss dweiss closed this as completed Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants