Do not return early from ThreadService#terminate#7633
Do not return early from ThreadService#terminate#7633headius merged 1 commit intojruby:jruby-9.3from
Conversation
|
Originally, I thought this was a continued problem with the Timeout module but I think it simply has the privileged distinction of being the only library thread we use that doesn't clean up after itself before termination. |
|
Hey Justin, this small change indeed makes sense. |
Previously, if we encounter an adopted thread when terminating the ThreadService we will return early and fail to finish killing threads we spawned. This changes the `return` to `continue` which I believe was always the intent.
justinstoller
force-pushed
the
fix_return_in_terminate_thread_service
branch
from
93809c6 to
5638b76
Compare
|
Sure thing @kares , I just wanted to get it up and see what folks thought before doing any more with it. Retargeted, let me know if you'd like me to do anything else! |
|
Also, is there an ETA on a version of 9.4 with this fix? Knowing a rough estimate would help judge if we should start internally testing with 9.4 now and be able to upgrade to a version with this fix before we release, or if we'd be stuck having to back it out. |
Since, 9.4.1 was just released 9.4.2 (and 9.3.11) might be several weeks away. Would a (nightly) snapshot build, with this fix included, be usable for you? |
|
It won't be 8 weeks, but it will be more than two. Depends on how critical and how many issues we get in the next week or two. Looking at this now. |
|
Yeah this is the right fix. Old code, but I'm surprised this hasn't been reported previously. I will merge this into 9.3 and forward to master and it will be in snapshots soon and releases a little less soon. |
|
Thanks! No need on the nightly build. FWIW, we certainly have a fairly unique use case with Puppet Server. We also are still carrying a work around for #6127, the removal of which caused us to find this. |
JRuby's terminate function will premeturely stop shutting down threads if it comes across an adopted thread. This will cause threads not properly shutdown by the time a JRuby instance terminates to be leaked. This may happen with code in the wild. Within Puppet Server it only happens with the Timeout module which is poorly behaved in this regard. This update should allow us to begin testing JRuby 9.4.x and Ruby 3.x compatability in Puppet Enterprise now, but we should not Puppet Server with this change. We must either take up the fix to this thread leakage, merged in jruby/jruby#7633 slated to be released in JRuby 9.4.1.1 or 9.4.2.0 depending on the number and severity of additional bugs reported, or revert this update. Note: the JRuby team estimates 2 - 8 weeks between when the above PR was merged and a new release containing it, which would be sometime between late February to early April 2023.
3 participants
Previously, if we encounter an adopted thread when terminating the ThreadService we return early and fail to finish killing threads we spawned.
This changes the
returntocontinuewhich I believe was always the intent.