Remove AtExit code #841

fzakaria · 2020-01-27T05:08:09Z

Following #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.

This would be considered a breaking change since the API surface is being modified.
I don't expect consumers of concurrent-ruby however to have used it. The reliance on the :auto-terminate option continues to work.

Conceptually, relying on the fact that threads are marked as daemon is much simpler & maps closer to the ExecutorService API which the library seems to model itself after (or the Java concurrent libraries in general).

Information regarding what it means to mark a thread as daemon:

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:

The exit method of class Runtime has been called and the security manager has permitted the exit operation to take place.

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.

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

Thanks for following up! I've added few related cleanups into the PR.

CHANGELOG.md

lib/concurrent-ruby/concurrent/configuration.rb

- disable_at_exit_handlers! has to be kept as noop not to break current usages - cleanup auto_terminate handling - remove ns_auto_terminate attribute - deprecate auto_terminate=

@LikeLakers2

Add @LikeLakers2 suggestion. Co-Authored-By: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com>

Fix grammatical error in configuration.rb

fzakaria · 2020-02-03T21:57:39Z

@pitr-ch looking the failures the builds are passing but not terminating.
Looks like one of the JRuby threads are not daemonizing.

lib/concurrent-ruby/concurrent/executor/java_single_thread_executor.rb

fzakaria · 2020-02-03T22:54:44Z

Sweet the Travis CI build passed with my latest commit @pitr-ch

lib/concurrent-ruby/concurrent/executor/java_executor_service.rb

fzakaria · 2020-02-05T20:02:57Z

spec/concurrent/executor/cached_thread_pool_spec.rb

@@ -212,6 +212,8 @@ module Concurrent
              pid       = spawn RbConfig.ruby, test_file
              Process.waitpid pid
              expect($?.success?).to eq true
+            rescue Errno::ECHILD
+              # child already gone


Ah! I was also fixing this :)

rescue Errno::ECHILD # the child could quit out fast enough before we try to wait for it!

I think we could TODO move this test to executor_service_shared & have pool_quits start every type of executor service: cached & single executor

yeah that sounds like a good idea! Would you be willing to do it?

fzakaria · 2020-02-05T23:26:31Z

Failure does not seem related -- i re-triggered build with empty commit.

Failures:
  1) Concurrent::Map updates dont block reads
     Failure/Error: expect(joined_thread).not_to eq nil
     
       expected: value != nil
            got: nil
     
       (compared using ==)

...
     # ./spec/concurrent/map_spec.rb:268:in `block in Concurrent'

pitr-ch · 2020-02-06T14:23:49Z

Thanks. I will remove the last commit and merge. (You can also trigger the build again by amending the last commit and force by force-pushing.)

fzakaria · 2020-02-06T15:54:16Z

Just squash merge it @pitr-ch ?

pitr-ch · 2020-02-06T16:24:19Z

Yeah that works as well, I planned git reset --hard HEAD^; git push -f. I won't get to it until tomorrow though.

fzakaria · 2020-02-06T16:27:09Z

I just did that & it's using the previous failed check of that commit :(

fzakaria · 2020-02-06T16:27:58Z

Had to amend the HEAD commit anyways

pitr-ch · 2020-02-07T20:40:28Z

Thanks for the PR Farid!

fzakaria · 2020-02-07T21:30:24Z

thanks @pitr-ch
I'm enjoying working on this project.
Is there another issue you think worth looking at to help?

pitr-ch · 2020-02-10T15:14:05Z

Awesome, thanks for the offer 👍 This project definitely needs more contributors! This #796 deserves some attention. Let me know if that interests you and I will assign it to you. If not please have a look at https://github.com/ruby-concurrency/concurrent-ruby/issues?utf8=%E2%9C%93&q=is%3Aissue+is%3Aopen+label%3Alooking-for-contributor

fzakaria · 2020-02-10T17:24:49Z

@pitr-ch on it.

…e test helper.rb since it now has no effect, see ruby-concurrency/concurrent-ruby#841

* Add rubocop-minitest * Run bundle exec rubocop --only Minitest/GlobalExpectations -a * Revert rubocop-minitest * Manually fix unit test errors * Remove deprecated Concurrent.disable_at_exit_handlers! from acceptance test helper.rb since it now has no effect, see ruby-concurrency/concurrent-ruby#841 refs: #4110 refs: #4116 pr: #5654

Concurrent: [DEPRECATED] Method #disable_at_exit_handlers! has no effect since it is no longer needed, see ruby-concurrency/concurrent-ruby#841

fzakaria requested review from pitr-ch and headius January 27, 2020 05:08

pitr-ch approved these changes Feb 2, 2020

View reviewed changes

LikeLakers2 reviewed Feb 2, 2020

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

fzakaria commented Feb 2, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/configuration.rb Outdated Show resolved Hide resolved

fzakaria commented Feb 2, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/configuration.rb Outdated Show resolved Hide resolved

fzakaria commented Feb 2, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/configuration.rb Outdated Show resolved Hide resolved

fzakaria commented Feb 2, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/configuration.rb Outdated Show resolved Hide resolved

pitr-ch force-pushed the remove-at-exit branch from b570725 to 138fe59 Compare February 2, 2020 20:46

pitr-ch and others added 4 commits February 3, 2020 09:53

auto_termination related cleanups

8956b90

- disable_at_exit_handlers! has to be kept as noop not to break current usages - cleanup auto_terminate handling - remove ns_auto_terminate attribute - deprecate auto_terminate=

Update CHANGELOG.md

2dd2101

Add @LikeLakers2 suggestion. Co-Authored-By: MichiRecRoom <1008889+LikeLakers2@users.noreply.github.com>

Update lib/concurrent-ruby/concurrent/configuration.rb

7557b70

Fix grammatical error in configuration.rb

Update lib/concurrent-ruby/concurrent/configuration.rb

9800707

Fix grammatical error in configuration.rb

pitr-ch force-pushed the remove-at-exit branch from 138fe59 to 9800707 Compare February 3, 2020 08:53

Add daemon thread factory for SingleThreadExecutor

af3a1f0

fzakaria commented Feb 3, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/executor/java_single_thread_executor.rb Show resolved Hide resolved

fzakaria requested a review from pitr-ch February 3, 2020 23:06

pitr-ch added 3 commits February 5, 2020 20:22

remove forgotten fulfilled todo

19b1a7e

DaemonThreadFactory is private

9426e88

Do not set auto_terminate twice

9cfbd78

fzakaria commented Feb 5, 2020

View reviewed changes

lib/concurrent-ruby/concurrent/executor/java_executor_service.rb Show resolved Hide resolved

Allow the child process to be already gone

f2de94a

fzakaria commented Feb 5, 2020

View reviewed changes

fzakaria force-pushed the remove-at-exit branch from 0230d44 to c94b781 Compare February 6, 2020 16:26

fzakaria force-pushed the remove-at-exit branch from c94b781 to 40bd29a Compare February 6, 2020 16:27

Moved auto_terminate daemon test to executor_service_shared.rb

2c13ac6

fzakaria force-pushed the remove-at-exit branch from 40bd29a to 2c13ac6 Compare February 6, 2020 18:25

pitr-ch merged commit 8ccb214 into ruby-concurrency:master Feb 7, 2020

quartzmo added a commit to quartzmo/google-cloud-ruby that referenced this pull request Apr 22, 2020

Remove deprecated Concurrent.disable_at_exit_handlers! from acceptanc…

c4d0ea0

…e test helper.rb since it now has no effect, see ruby-concurrency/concurrent-ruby#841

sj26 mentioned this pull request Aug 25, 2020

Run GoodJob on puma boot bensheldon/good_job#91

Closed

st0012 mentioned this pull request Nov 9, 2021

Synchronously drain async error reporting queue getsentry/sentry-ruby#1612

Closed

itkrt2y mentioned this pull request Dec 20, 2023

Remove deprecated auto_terminate option joker1007/rukawa#52

Merged

adamruzicka added a commit to adamruzicka/dynflow that referenced this pull request Jan 26, 2024

Disable concurrent at exit handlers

0359cde

Concurrent: [DEPRECATED] Method #disable_at_exit_handlers! has no effect since it is no longer needed, see ruby-concurrency/concurrent-ruby#841

adamruzicka added a commit to adamruzicka/dynflow that referenced this pull request Jan 30, 2024

Disable concurrent at exit handlers

bbb411e

Concurrent: [DEPRECATED] Method #disable_at_exit_handlers! has no effect since it is no longer needed, see ruby-concurrency/concurrent-ruby#841

adamruzicka added a commit to adamruzicka/dynflow that referenced this pull request Jan 31, 2024

Disable concurrent at exit handlers

85f98cf

Concurrent: [DEPRECATED] Method #disable_at_exit_handlers! has no effect since it is no longer needed, see ruby-concurrency/concurrent-ruby#841

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove AtExit code #841

Remove AtExit code #841

fzakaria commented Jan 27, 2020 •

edited

pitr-ch left a comment

fzakaria commented Feb 3, 2020

fzakaria commented Feb 3, 2020

fzakaria Feb 5, 2020

pitr-ch Feb 5, 2020

fzakaria Feb 5, 2020

fzakaria commented Feb 5, 2020

pitr-ch commented Feb 6, 2020

fzakaria commented Feb 6, 2020

pitr-ch commented Feb 6, 2020

fzakaria commented Feb 6, 2020

fzakaria commented Feb 6, 2020

pitr-ch commented Feb 7, 2020

fzakaria commented Feb 7, 2020

pitr-ch commented Feb 10, 2020

fzakaria commented Feb 10, 2020

Remove AtExit code #841

Remove AtExit code #841

Conversation

fzakaria commented Jan 27, 2020 • edited

pitr-ch left a comment

Choose a reason for hiding this comment

fzakaria commented Feb 3, 2020

fzakaria commented Feb 3, 2020

fzakaria Feb 5, 2020

Choose a reason for hiding this comment

pitr-ch Feb 5, 2020

Choose a reason for hiding this comment

fzakaria Feb 5, 2020

Choose a reason for hiding this comment

fzakaria commented Feb 5, 2020

pitr-ch commented Feb 6, 2020

fzakaria commented Feb 6, 2020

pitr-ch commented Feb 6, 2020

fzakaria commented Feb 6, 2020

fzakaria commented Feb 6, 2020

pitr-ch commented Feb 7, 2020

fzakaria commented Feb 7, 2020

pitr-ch commented Feb 10, 2020

fzakaria commented Feb 10, 2020

fzakaria commented Jan 27, 2020 •

edited