-
-
Notifications
You must be signed in to change notification settings - Fork 925
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
Running threads stay running after teardown #3313
Comments
9k fix is sufficient, or are you running 1.7.x? |
We run 1.7.x under torquebox 3. |
I've fixed similar issues on several gems when I was running 1.7.x. I've found the following approach works well: |
Hey @nirvdrum I feel like jRuby should do this automaticlly. I would not trust a lot of MRI developers to think in an embedded way. https://github.com/mongodb/mongo-ruby-driver/blob/master/lib/mongo/server/monitor.rb is the example that got me here. How many gems have you fixed up? |
@stewartmatheson So to clarify, do you believe you just need the threads removed from the thread list? My main concern here is that running threads may still reference the runtime and may attempt to access resources that have been shut down. In a related situation, we have discussed various ways to keep the runtime alive even if the main thread exits but have never decided on one. For example, we might provide JRuby.main_thread_shutdown = false or something. I'd like to understand your use case a bit better. Why is the runtime shutting down when there's still threads running that might access it? Are they external Java threads that were "adopted" into the runtime, or are they Ruby threads? Why can't you shut down the threads yourself? |
@stewartmatheson I've had to fix at least a half dozen gems. While frustrating, everyone has been happy to accept the change at the very least. I'm hoping it ends up becoming a common pattern. |
@headius The problem is a lot of gems spin up background threads for various things. They're never explicitly stopped -- they expect the thread to just get killed when the process dies. JRuby's shutdown mechanism doesn't forcibly kill running threads, so those threads continue to run forever. In a hot deployment environment like TorqueBox's, this amounts to a leak of runtimes. Instead of having one copy of the application running, you end up with N, since the background threads are enough to keep the entire old runtime around. |
Hi @headius. Yes my understanding is that if I where to clear the ruby thread map then the threads would no longer run. disposeCurrentThread() in ThreadService removes the Tread.currentThread() so I assumed this was how treads would be disposed. Is this assumption wrong? As for the use case @nirvdrum described the exact issue. What's at play here for me is a disconnect between MRI and Jruby embedded. In my head a Jruby embedded instance should be analogous to an OS level MRI process. All threads will get terminated by the OS when the MRI process dies thus all threads should get termniated by tearDown() when a Jruby instance "dies". Would there be a use case for keeping any threads running? |
we do the threads issue sounds similar to the classloader issue for me. |
A few clarifications:
So tldr: I think it might be valid to signal all JRuby threads associated with a given JRuby instance that they should attempt to shut down, but only when running as an embedded JRuby. Now, on the practical side...I'm not sure what this should look like. JRuby itself doesn't know if it's embedded. We guess at it by checking if we're run from Main.main, but that's not super reliable. I'd be in favor of adding a blessed Bumping to 9.1 for further discussion. |
Maybe add a new runtime teardown method that is invoked on SIGTERM and treat the existing one as the embedded case? |
Pinging @bbrowning, since he probably has some ideas. |
We can leave this open for discussion a bit more, but I'm not clear that there's anything we should be doing. There's ways to go manually kill all the threads, and if you're walking away from them you probably should clean them up. There may be justification for a new feature to shut down all threads, but it's definitely not fleshed out enough for 9.1. |
The problem is most often they're not threads you directly manage. They're background threads started up by a gem. I guess you could iterate through |
@headius Can Jruby have a flag so it knows if it's running embedded? Could this be passed in somehow on startup? This way you could have one tearDown method that could act in what ever way it needs to depending on if it's embedded or not. |
@stewartmatheson We do have a way to detect this... I guess another option here would be to provide a separate tearDown you can call yourself to clean up all threads it has encountered, but I still don't have a solution for this I like :-\ Bumping to 9.2 unless we come up with a good idea. |
Detargeted since this is still an open question and the fix is not clear. |
This change clears the following on JRuby runtime teardown: * The thread-local context reference in ThreadService (dereferenced entirely) * The mainContext reference in ThreadService * The mapping from native threads to RubyThread instances. This should clear all JRuby-specific runtime data attached to both Ruby and adopted Java threads. Fixes jruby#3313. Supersedes jruby#3316.
I've created a new version of #3316 that also clears the localContext thread-local variable, to avoid keeping ThreadContext instances alive for threads that JRuby has seen, and the mainContext variable, to ensure the context associated with the "main" thread does not linger longer than it should. That commit should fix this bug but it should be reviewed. |
New PR is #5390. |
This change clears the following on JRuby runtime teardown: * The thread-local context reference in ThreadService (dereferenced entirely) * The mainContext reference in ThreadService * The mapping from native threads to RubyThread instances. This should clear all JRuby-specific runtime data attached to both Ruby and adopted Java threads. Fixes jruby#3313. Supersedes jruby#3316.
This has come up many times; Ruby threads are daemon threads by default, which means they will not keep the main script from exiting. As a result, if there's any threads still runnign when the main script exits, we may tear down resources they need to run properly. Even though they're about to die, they may produce errors on the way out as shown in jruby#5909 (comment) and other bug reports over the years like jruby#5519, jruby#3316, jruby#3313 and others. This is not a fix for the behavior, but it introduces a non-verbose warning if the JRuby runtime is torn down while there's still active threads.
Note #6176 attempts to implement part of the thread shutdown requested in this issue. Specifically, any non-adopted thread (threads started by JRuby from Ruby code using |
I would expect when I call tearDown on a ruby instance it would destroy any running ruby threads. Running threads however stay running which leads to memory leaks as the JVM will never GC objects with references in these running threads. This becomes a real issue when using servers such as torquebox that keep the same JVM running while destroying and creating ruby instances.
On a sidenote: I am looking for a way to get involved in the JRuby community. I have loved using JRuby for 2 years now and I am keen to give some of my time up to help out. I am having a bit of trouble figuring out where to start though. If this change is possible I would love it if some one could mentor me though this patch to get me started. Nothing too big just a quick chat session or even a 5 min skype call.
The text was updated successfully, but these errors were encountered: