Introduce DaemonThreadFactory #840

fzakaria · 2020-01-08T18:09:28Z

Introduce a ThreadFactory that will set Thread#setDaemon according
to the auto_terminate value.

This makes the need for the AtExit functionality uneeded since all
threads can now be started daemonized.

Furthermore, the gross hack of Configuration#disable_at_exit_handlers!
is especially not needed since this allows the JVM to properly terminate
even with minitest.

You can test this with:

ruby test/concurrent/executor/test_cached_thread_pool.rb

This fixes issues reported by others wherein the JVM fails to shutdown when used with Minitest.

fixes #839
fixes #817

fzakaria · 2020-01-08T18:10:00Z

tag @kaikuchn @headius

fzakaria · 2020-01-08T18:11:39Z

With this change I would rip out the AtExit hack completely.
All threads by default in Ruby are daemonized and concurrent-ruby should respect this by default.

Anyone who uses the library and sets auto_terminate to false, is then responsible for termination themselves.

fzakaria · 2020-01-08T18:14:17Z

test/concurrent/executor/test_cached_thread_pool.rb

@@ -0,0 +1,14 @@
+$LOAD_PATH.unshift File.expand_path("../../../lib", __dir__)


Unfortunately, testing the functionality is only possible with minitest.
You can see the comment surround disable_at_exit_handlers! to read why it can't be tested in RSpec.

For now I've included a single raw minitest file -- I'm open to suggestion as to how best to incorporate it into the Rakefile according to this build process.

It would seem better to spin off a separate process and ensure that it terminates within some timeout. The test is fine but I think it should run in its own subprocess.

There were tests that essentially did that in RSpec -- the failure however was that Minitest was messing with the AtExit handlers.
Given that it's a very popular test framework; I thought it would be reasonable to incorporate it sparingly for such cases. I'm not too strongly attached one way or another.

Could we instead start a simple process that messes with the AtExit handlers? That might be easier to understand and we wouldn't introduce another development dependency. WDYT?

A sub-process could be written which calls disable_at_exit_handlers!
Personal opinion on the matter though is that a test that verifies that it works with minitest is pretty useful given the popularity of the gem.

This PR has seem some churn -- I'd love to get this fix in and follow-up with ammendments/improvements to the testing facility also ?

That would also play well with the idea I've been chatting about on gitter channel about even potentially removing AtExit completely. (It's uneeded now if all the threads are daemon)

fzakaria · 2020-01-08T19:52:51Z

The Travis CI failure seems to be related to use of Unsafe and not my code change.

headius · 2020-01-13T23:11:02Z

I pushed a tweak to the travis configs to use JRuby 9.2.9.0 instead of 9.2.8.0, which seems to have fixed the two hanging "latest" builds.

fzakaria · 2020-01-13T23:12:10Z

@headius can you restart the travis ?
I'd love to get this in + whatever changes necessary.

Seems pretty high considering it makes using Minitest & this library a PITA

headius · 2020-01-13T23:13:58Z

@fzakaria I'm looking at master to try to get everything green there, but with at least JRuby 9.2.9 and 9.1.17 green I think we'd be ok to merge this in. Could you rebase your PR on master and repush, just so I can confirm 9.2.9 on Java 8 still passes?

There are still failures for JRuby 9.2.9 on Java 11 and for other JRuby versions (on master) that aren't your fault.

headius · 2020-01-13T23:14:55Z

FWIW I do not have push rights for the gem, so I'll need help from @pitr-ch or @jdantonio to get a release out.

fzakaria · 2020-01-13T23:29:13Z

Sounds good -- I'll rebase in ~30 minutes. Going to pick up my son + few other errands
etc..

As an aside; if this gets merged; what do you think of removing most of the AtExit code ?
Seems that it will be superseded by just making all the threads daemon

headius · 2020-01-13T23:42:45Z

@fzakaria Possibly... I have to inspect that logic a bit more closely before I make a decision. The key point will be whether these daemon threads need any sort of at_exit cleanup. If they don't and they can just die, then it's probably fine.

There may still be a need for at_exit logic in other places, though.

Introduce a ThreadFactory that will set `Thread#setDaemon` according to the _auto_terminate_ value. This makes the need for the `AtExit` functionality uneeded since all threads can now be started daemonized. Furthermore, the gross **hack** of `Configuration#disable_at_exit_handlers!` is especially not needed since this allows the JVM to properly terminate even with minitest. You can test this with: ``` ruby test/concurrent/executor/test_cached_thread_pool.rb ``` fixes ruby-concurrency#839 fixes ruby-concurrency#817

fzakaria · 2020-01-14T00:13:23Z

@headius just rebased

We can move the discussion else where regarding AtExit if there's an IRC / Slack also.

My thoughts on the matter are highly influenced from Java's ExecutorService which does not have a "auto clean shutdown" -- rather individually the user is responsible for adding a shutdown hook themselves.

concurrent-ruby seems heavily inspired by Java's API; so leaving that up to the user sounds reasonable and avoids nasty problems light this in the library
(where another library such as minitest overrides the shutdown behavior)

headius · 2020-01-14T01:17:25Z

We can move the discussion else where regarding AtExit if there's an IRC / Slack also.

There's an active concurrent-ruby channel on Gitter: https://gitter.im/ruby-concurrency/concurrent-ruby. Let's discuss there.

headius · 2020-01-14T14:06:23Z

Strangely, the JRuby 9.2.9 on Java 11 suite passes just fine on my local system (MacOS).

fzakaria · 2020-01-14T19:37:23Z

The build failures aren't that easy to grok however they don't look to be related to the change AFAICT @headius .

jdantonio · 2020-01-14T20:37:13Z

@fzakaria The failing test has a comment FIXME this leads to weird message processing ordering and is one of the "edge" features. So this may just be a buggy test and probably shouldn't prevent us from merging. Can you please comment out the test on line 191 of spec/concurrent/actor_spec.rb and see if it passes CI? If that works we may just temporarily disable the test and merge your change.

spec/concurrent/actor_spec.rb

fzakaria · 2020-01-15T03:44:29Z

Okay @headius & @jdantonio looks like JDK 11 build is just FUBAR
It's failing for other reasons.

TypeError: illegal access on 'isShutdown': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) 
cannot access a member of class java.util.concurrent.Executors$DelegatedExecutorService (in module java.base) with modifiers "public"
        ns_shutdown? at /home/travis/build/ruby-concurrency/concurrent-ruby/lib/concurrent/executor/java_executor_service.rb:70
         ns_running? at /home/travis/build/ruby-concurrency/concurrent-ruby/lib/concurrent/executor/java_executor_service.rb:58

I think it should be moved to allowed failures section in the CI

headius · 2020-01-15T15:08:16Z

I believe the Java 11 errors are due to a bug in JRuby 9.2.9 that's already fixed for 9.2.10.

headius · 2020-01-15T15:32:19Z

Confirmed: JRuby 9.2.9 (on Java 11) presents the same errors and hangs the same way locally, while JRuby 9.2.10 runs to completion every time.

I believe it is safe to disregard the JRuby 9.2.9 on Java 11 failure.

headius · 2020-01-15T15:51:07Z

I have added concurrent-ruby to JRuby's travis-ci run to make sure it stays green in the future: https://travis-ci.org/jruby/jruby/jobs/637495980

headius

The spec skips and removals should be pulled out of this PR and put into a separate one; fixes to flaky tests are not relevant to this PR. Otherwise it looks fine.

If you need those skips because you want this to be green before merging, just make the other PR and we'll get that in first. Then you can rebase.

lib/concurrent/executor/cached_thread_pool.rb

headius · 2020-01-15T15:55:01Z

test/concurrent/executor/test_cached_thread_pool.rb

@@ -0,0 +1,14 @@
+$LOAD_PATH.unshift File.expand_path("../../../lib", __dir__)


It would seem better to spin off a separate process and ensure that it terminates within some timeout. The test is fine but I think it should run in its own subprocess.

pitr-ch · 2020-01-19T21:21:56Z

master changed file layout, I've rebased this PR for you and force pushed.

…ISSUE-839

fzakaria · 2020-01-20T01:20:06Z

spec/concurrent/executor/cached_thread_pool_spec.rb

+        # https://github.com/ruby-concurrency/concurrent-ruby/issues/839
+        it 'does not stop shutdown ' do
+          Timeout::timeout(10) do
+            `


@pitr-ch I re-worked the test and have it running via spec.

I validated that this test on master causes the test to hang.
Unfortunately, I can't seem to get it to gracefully fail even with Timeout. The timeout error raising Kernel#raise does not seem to interrupt it waiting for the subprocess. Not sure how much it matters since we expect the test to pass...

note: You can't use Process#fork on JRuby

fzakaria · 2020-01-20T17:02:48Z

The JRuby head failure does not seem related to the pull-request:
rspec ./spec/concurrent/atomic/event_spec.rb:93 # Concurrent::Event#wait returns immediately when the event has been set

(Could be from the rebase from master?)

headius · 2020-01-24T17:56:41Z

Green!

fzakaria · 2020-01-24T18:17:25Z

@pitr-ch appreciate a release when you can.

pitr-ch · 2020-01-26T10:38:57Z

Thanks again for the fix. I am doing pre-release this weekend and a release probable next one.

pitr-ch · 2020-01-26T15:52:21Z

pre-release is out

Following ruby-concurrency#840 which allowed all threads created by concurrent-ruby to be marked as daemon -- specifically in JRuby; the "AtExit" handling is no longer needed. Delete all references to "AtExit" and replace it with clearer documentation that the users can select whether the threads started shall be marked as daemon threads. > When a Java Virtual Machine starts up, there is usually a single non-daemon thread (which typically calls the method named main of some designated class). > The Java Virtual Machine continues to execute threads until either of the following occurs: > > 1. The exit method of class Runtime has been called and the security manager has permitted the exit operation to take place. > 2. All threads that are not daemon threads have died, either by returning from the call to the run method or by throwing an exception that propagates beyond the run method.

pitr-ch · 2020-02-16T19:08:48Z

released in 1.1.6

headius · 2020-02-16T19:54:02Z

@pitr-ch Thank you!

fzakaria mentioned this pull request Jan 8, 2020

Pool thread not daemon jruby/jruby#6022

Closed

fzakaria commented Jan 8, 2020

View reviewed changes

fzakaria force-pushed the ISSUE-839 branch from 57de5b8 to 94fd5d8 Compare January 14, 2020 00:08

fzakaria requested review from pitr-ch and headius January 14, 2020 19:37

Skip test in actor_spec that leadas to inconsistent failures

e6f39fd

fzakaria commented Jan 14, 2020

View reviewed changes

spec/concurrent/actor_spec.rb Outdated Show resolved Hide resolved

fzakaria added 3 commits January 14, 2020 14:50

Skip another test

27d9962

Skip all actor tests for JDK 11

276854a

Disable all the tests in spec_actor.rb

8a858e4

headius requested changes Jan 15, 2020

View reviewed changes

fzakaria and others added 6 commits January 19, 2020 22:16

Skip another test

81ddbe1

Skip all actor tests for JDK 11

dc82f8d

Disable all the tests in spec_actor.rb

85c4163

Reverted actor_spec & added minitest to rakefile

c39f189

Style feedback from @kaikuchn

6f1996d

Fix file placement after rebasing on master

5f8d546

pitr-ch force-pushed the ISSUE-839 branch from b50d44f to 5f8d546 Compare January 19, 2020 21:21

fzakaria added 5 commits January 19, 2020 16:27

Merge branch 'ISSUE-839' of github.com:fzakaria/concurrent-ruby into …

4e221ae

…ISSUE-839

rm common file

49e92d3

reverted Rakefile & added spec for subproces

dc50280

remvoed minitest

196e3a3

cleanup test

c08d0cf

fzakaria commented Jan 20, 2020

View reviewed changes

fzakaria requested a review from pitr-ch January 20, 2020 01:20

pitr-ch approved these changes Jan 26, 2020

View reviewed changes

Make sure we run what we expect

86bf86c

pitr-ch force-pushed the ISSUE-839 branch from c39dadf to 86bf86c Compare January 26, 2020 10:06

pitr-ch merged commit 18f96b7 into ruby-concurrency:master Jan 26, 2020

fzakaria mentioned this pull request Jan 27, 2020

Remove AtExit code #841

Merged

pitr-ch mentioned this pull request Feb 16, 2020

illegal access on 'newThread': class org.jruby.javasupport.JavaMethod (in module org.jruby.dist) cannot access a member of class java.util.concurrent.Executors$DefaultThreadFactory (in module java.base) with modifiers "public" #850

Closed

Adithya-copart mentioned this pull request Mar 17, 2020

Using at_exit and Signal.trap possibly creates a race-condition jruby/jruby#5437

Closed

		@@ -0,0 +1,14 @@
		$LOAD_PATH.unshift File.expand_path("../../../lib", __dir__)

Introduce DaemonThreadFactory #840

Introduce DaemonThreadFactory #840

Uh oh!

Conversation

fzakaria commented Jan 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fzakaria commented Jan 8, 2020

Uh oh!

fzakaria commented Jan 8, 2020

Uh oh!

fzakaria Jan 8, 2020

Choose a reason for hiding this comment

Uh oh!

headius Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

fzakaria Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

kaikuchn Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

fzakaria Jan 16, 2020

Choose a reason for hiding this comment

Uh oh!

fzakaria commented Jan 8, 2020

Uh oh!

headius commented Jan 13, 2020

Uh oh!

fzakaria commented Jan 13, 2020

Uh oh!

headius commented Jan 13, 2020

Uh oh!

headius commented Jan 13, 2020

Uh oh!

fzakaria commented Jan 13, 2020

Uh oh!

headius commented Jan 13, 2020

Uh oh!

fzakaria commented Jan 14, 2020

Uh oh!

headius commented Jan 14, 2020

Uh oh!

headius commented Jan 14, 2020

Uh oh!

fzakaria commented Jan 14, 2020

Uh oh!

jdantonio commented Jan 14, 2020

Uh oh!

Uh oh!

fzakaria commented Jan 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

headius commented Jan 15, 2020

Uh oh!

headius commented Jan 15, 2020

Uh oh!

headius commented Jan 15, 2020

Uh oh!

headius left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

headius Jan 15, 2020

Choose a reason for hiding this comment

Uh oh!

pitr-ch commented Jan 19, 2020

Uh oh!

fzakaria Jan 20, 2020

Choose a reason for hiding this comment

Uh oh!

fzakaria commented Jan 20, 2020

Uh oh!

headius commented Jan 24, 2020

Uh oh!

fzakaria commented Jan 24, 2020

Uh oh!

pitr-ch commented Jan 26, 2020

Uh oh!

fzakaria commented Jan 8, 2020 •

edited

Loading

fzakaria commented Jan 15, 2020 •

edited

Loading