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

Running threads stay running after teardown #3313

Closed
stewartmatheson opened this issue Sep 7, 2015 · 20 comments
Closed

Running threads stay running after teardown #3313

stewartmatheson opened this issue Sep 7, 2015 · 20 comments
Labels
Milestone

Comments

@stewartmatheson
Copy link
Contributor

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.

@headius
Copy link
Member

headius commented Oct 2, 2015

9k fix is sufficient, or are you running 1.7.x?

@stewartmatheson
Copy link
Contributor Author

We run 1.7.x under torquebox 3.

@nirvdrum
Copy link
Contributor

nirvdrum commented Oct 3, 2015

I've fixed similar issues on several gems when I was running 1.7.x. I've found the following approach works well:

wr0ngway/lumber@4abd5a8

@stewartmatheson
Copy link
Contributor Author

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?

@headius
Copy link
Member

headius commented Oct 7, 2015

@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?

@nirvdrum
Copy link
Contributor

nirvdrum commented Oct 7, 2015

@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.

@nirvdrum
Copy link
Contributor

nirvdrum commented Oct 7, 2015

@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.

@stewartmatheson
Copy link
Contributor Author

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?

@mkristian
Copy link
Member

we do close() the jruby-classloader on tearDown() of the ScriptingContainer but do not close it when jruby got executed via the command since there are cases where those classes were still used after the "teardown", i.e. ruby-processing #3152

the threads issue sounds similar to the classloader issue for me.

@headius
Copy link
Member

headius commented Jan 21, 2016

A few clarifications:

  • Ruby threads started by JRuby are marked as "daemon" in the JVM. They will not prevent the JVM from shutting down, but we don't explicitly tell them to terminate. If the only remaining JVM threads are JRuby threads, the JVM will shut down.
  • The situation for an embedded JRuby is trickier. JRuby threads still running will remain running, because we don't actively attempt to kill them. This might be reasonable to do. However, I think it's only valid for the embedded case. MRI does NOT give child threads any indication that the process is shutting down; if we were to do that we'd get all sorts of ugly noise and exceptions raised every time a JRuby process ends.
  • Threads started by other libraries or in Java code will generally not be marked as daemon threads, but they are also not our responsibility. If we attempted to kill all JVM threads that touch JRuby when a JRuby instance shuts down, we could end up killing things like Swing event threads or RMI messaging threads.

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 org.jruby.Ruby.signalThreads for embedders, but I need to be convinced that JRuby itself should actively do anything.

Bumping to 9.1 for further discussion.

@headius headius modified the milestones: JRuby 9.1.0.0, JRuby 9.0.5.0 Jan 21, 2016
@nirvdrum
Copy link
Contributor

Maybe add a new runtime teardown method that is invoked on SIGTERM and treat the existing one as the embedded case?

@nirvdrum
Copy link
Contributor

Pinging @bbrowning, since he probably has some ideas.

@headius
Copy link
Member

headius commented Apr 15, 2016

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.

@headius headius modified the milestones: JRuby 9.1.1.0, JRuby 9.1.0.0 Apr 15, 2016
@nirvdrum
Copy link
Contributor

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 Thread.list and kill. Deferring is fine, but I think it should be a feature of some sort for the runtime teardown.

@stewartmatheson
Copy link
Contributor Author

@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.

@headius headius modified the milestones: JRuby 9.1.1.0, JRuby 9.1.2.0 May 11, 2016
@enebo enebo modified the milestones: JRuby 9.1.2.0, JRuby 9.1.3.0 May 23, 2016
@headius
Copy link
Member

headius commented Aug 17, 2016

@stewartmatheson We do have a way to detect this... runtime.getInstanceConfig().isHardExit indicates that JRuby was started from the command line, and should "hard exit" the whole process when e.g. Kernel#exit is called.

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.

@headius headius modified the milestones: JRuby 9.2.0.0, JRuby 9.1.3.0 Aug 17, 2016
@headius headius removed this from the JRuby 9.2.0.0 milestone May 15, 2018
@headius
Copy link
Member

headius commented May 15, 2018

Detargeted since this is still an open question and the fix is not clear.

headius added a commit to headius/jruby that referenced this issue Oct 30, 2018
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.
@headius
Copy link
Member

headius commented Oct 30, 2018

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.

@headius
Copy link
Member

headius commented Oct 30, 2018

New PR is #5390.

headius added a commit to headius/jruby that referenced this issue Oct 30, 2018
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.
@enebo enebo added this to the JRuby 9.2.1.0 milestone Nov 1, 2018
headius added a commit to headius/jruby that referenced this issue Oct 16, 2019
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.
@headius
Copy link
Member

headius commented Apr 14, 2020

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 Thread.new or second-hand paths like Process.detach) will be killed and joined at shutdown. Threads started outside of the JRuby runtime, including the main thread, are considered "adopted" and will not be killed or joined.

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

6 participants