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 · 19 comments

Comments

Projects
None yet
6 participants
@stewartmatheson
Copy link
Contributor

commented Sep 7, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 2, 2015

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

@stewartmatheson

This comment has been minimized.

Copy link
Contributor Author

commented Oct 3, 2015

We run 1.7.x under torquebox 3.

@nirvdrum

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 5, 2015

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 8, 2015

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

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

@nirvdrum

This comment has been minimized.

Copy link
Contributor

commented Jan 21, 2016

Pinging @bbrowning, since he probably has some ideas.

@headius

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Apr 16, 2016

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2016

@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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

Clear references to threads and thread contexts on teardown.
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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

commented Oct 30, 2018

New PR is #5390.

headius added a commit to headius/jruby that referenced this issue Oct 30, 2018

Clear references to threads and thread contexts on teardown.
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 closed this in #5390 Oct 31, 2018

@enebo enebo added this to the JRuby 9.2.1.0 milestone Nov 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.