Concurrent::RejectedExecutionError when running with minitest/autorun via JRuby

[iNecas]

I've been hitting Concurrent::RejectedExecutionError when running my test-stuite. It was ok with MRI, but switching to JRuby was the game-changer. It turns out there seems to be an at_exit trap that shuts down the Java thread pool. I've first tried to fix that using the same approach as in this test suite, reinitializing the Concurrent.configuration. However, there still was an issue with @root variable in actor keeping the reference to an old thread pool via Delayed (

concurrent-ruby/lib/concurrent/actor.rb

Lines 42 to 46 in 124ea7e

	@root = Delay.new do
	Core.new(parent: nil, name: '/', class: Root, initialized: ivar = IVar.new).reference.tap do
	ivar.no_error!
	end
	end

)

So I needed to reinitialize this as well the hackish way:

iNecas/dynflow@b9a7bf2

What do you think about having set of helpers that helps with setting the testing environment and this kind of issues (I guess there might be more: we're not using all the concurrent-ruby features.)

[jdantonio]

@iNecas We definitely need to figure out a better way to handle shutting down the Java thread pools.

The reason for the at_exit hook is that Java's thread pools will prevent the JVM from shutting down if the thread pool is not explicitly shut down. Under JRuby our thread pools are simple wrappers around the thread pools from java.util.concurrent and it's the underlying thread pool that causes the problem. When I first encountered this it took me a while to figure out what was happening so I grew concerned that users would have the same problem. The at_exit hook solved the problem by forcing the pool to be explicitly shut down. Our current Actor implementation came much later and that's when the at_exit hook became a problem.

I wonder if we're trying to solve this problem the wrong way. Rather than add extra layers around the thread pools, could we solve the problem allowing the creator of a given pool to not create the at_exit hook? Something like:

pool = Concurrent::FixedThreadPoool.new(5, set_shutdown_hook: false)

The new option would be ignored unless running under JRuby and the default would be true to ensure proper shutdown. It would then be a simple matter of updating the various calls to set_shutdown_hook (such as here to honor the true/false flag. The Actor framework could then create it's internal thread pool without the at_exit hook and then take responsibility for making sure the pool is shutdown properly. (And, of course, we would have to thoroughly document the ramifications of not enabling the shutdown hook.)

@pitr-ch Thoughts?

[pitr-ch]

It's also related to #227.

Thanks @iNecas for filling. (I know you did not have too since you've already told me over coffee.)

Thanks @jdantonio for the info, I did not know about https://github.com/ruby-concurrency/concurrent-ruby/search?q=set_shutdown_hook&type=Code&utf8=%E2%9C%93

I think the right approach could be: not installing any at_exit hooks by default to avoid any hacky re-initializations when something is terminated prematurely. The global thread-pools should be there available always. User created thread-pools are up to user to terminate properly, I like set_shutdown_hook: option (it should be honored on MRI too I think, better to shutdown properly then let MRI to kill the threads in random order [I've already saw issues with that.]). Still for convenience there should be something like Concurrent.auto_terminate! included which would work in simple use-cases. Which would make it change for v0.9.

What do you think?

[iNecas]

Nice catch with the set_shutdown_hook. I first thought that was something Java scpecific (as it wasn't happening on MRI). Definitely +1 to not do so automatically, especially not having different approaches in jruby vs. others.

Not having there the at_exit termination would definitely fix my testing issues

[jdantonio]

@pitr-ch @iNecas I believe that the at_exit hook should be created by default. To not do so will lead to a very bad experience for many users. The java.util.concurrent thread pools must be manually shut down within the application or the process will simply hang on exit, become completely unresponsive, and need manually killed by a user. Since we silently create the global thread pools without the user's knowledge we cannot expect our users to manually shut down these thread pools within their application. When a user manually creates a thread pool the danger is lessened since we can document the problem, but the failure condition is pretty severe. Assuming that our user's will always read the documentation is a big leap of faith.

If we add the set_shutdown_hook option and make the default true we'll protect the users who aren't deeply versed in concurrency while still giving the advanced users absolute control.

Rather than having the pure-Ruby thread pools ignore this flag, I think we should add the at_exit hook for all thread pools to make the behavior consistent. Adding the at_exit hook should have no practical effect when using pure-Ruby thread pools.

[jrochkind]

Under MRI, if you exit with running thread pools with current work in progress-- I think maybe it will actually kill threads and abandon work?

In general, I think dealing with thread pools you created at program termination, is something you're going to have to do yourself, just like you do in Java. You have to decide whether you want to wait on them, or shut them down messily, or what.

But I understand this isn't appropriate for the default built-in thread pools. I agree you need to come up with a way to handle them by default -- but do not think it should apply by default to any thread pool created by client code (not the default core pools created by concurrrent-ruby itself). I think you are better off forcing the user to recognize the problem and decide how to deal with it.

That is, I'd only default set_shutdown_hook to true on the built-in default thread pools, if you go that route.

Also, I agree that you should use the at_exit hook in MRI too, for consistency. You never know if you're thinking of all the edge cases, best to be consistent, if it doesn't appear to hurt.

[jdantonio]

@jrochkind Yes, under MRI when you exit the program the runtime will kill all the threads, regardless of what they are doing. You'll lose those tasks as well as any tasks on the queue that have not been started yet. And therein lies the problem. Under JRuby the thread pools will prevent the process from exiting unless they get properly shut down. The only way we can get that behavior on MRI would be to add an at_exit hook that prevents the application from closing.

If we want the shutdown behavior of all our thread pools (Java-backed and pure-Ruby) to be identical we must do something with at_exit. I don't see a way around that unless someone can figure something else out. Precisely what we do is the question.

If we make the default behavior of our thread pools be "no at_exit hook" then the Java-backed and pure-Ruby thread pools will behave differently. The Java thread pools will prevent the application from exiting but the pure-Ruby ones will not. If we want Java-like behavior from the pure-Ruby thread pools then we need to add an at_exit hook which causes the program to not exit unless all thread pools have been shut down. That would be a pretty intrusive change.

Sadly, I don't think we're going to find a perfect solution here. Every option seems to have a downside.

[iNecas]

I'm ok with both doing something at_exit to the default thread pools, or not, as far as it's consistent on all the implementations, ideally configurable. The problem for me was just, that it was working without issues on MRI, while I saw some strange errors on JRuby. From the adoption point of view, having the default shutdown makes sense.

[jrochkind]

Hmm, I see, yeah, thanks @jdantonio. That does make me understand why the original solution of an at_exit hook everywhere makes sense... but then the question is why it was causing a problem as reported in this issue. Does anyone have a handle on that?

The Actor framework could then create it's internal thread pool without the at_exit hook and then take responsibility for making sure the pool is shutdown properly.

And how would it do that, under MRI, without an at_exit hook?

[rkday]

@jrochkind - I think it's causing a problem because minitest/autorun does its auto-running magic through another at_exit hook, and the two hooks conflict.

[jrochkind]

Aha, right.

If multiple handlers are registered, they are executed in reverse order of registration.

A workaround might be messing with when minitest vs concurrent-ruby gets loaded, to try to get concurrent-ruby's at_exit to run last. But I realize that's not very satisfactory, I'm really just brainstorming here.

But I wonder if the current behavior is really mostly right, and we just need a special way to disable/change it in the special circumstances where it is not, like minitest/autorun. Maybe a way to just disable all the at_exit behavior in some cases, but everything is still registered somewhere, where you can manually call something like Concurrent.at_exit yourself, perhaps in your test_helper.rb -- or maybe in most test cases, you don't even need the at_exit behavior, and can just disable it and not worry about some workers being killed in progress (only in MRI), cause you were testing and those guys are lost if the tests are finished anyway.

Maybe it's good enough to just leave things as they are, but with a way to disable at_exit behavior entirely, which it could be recommended you do in your test_helper when using minitest/autorun (or other test autorunners using at_exit).

[jrochkind]

Wait, every single Executor adds it's own at_exit hook when it's created? That's what it looks like looking at the code.

It might make things more manageable (and performant?) if they added themselves to a queue or something, and there was a single global at_exit hook that killed everything in the queue. That'd make it easier to "turn off", and/or call yourself at program termination, "Okay, do all the stuff we're supposed to do before termination"

Still kind of just brainstorming, I'm thinking of implications already making it more complicated. Sorry if I'm just being annoying.