Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Align Java Executor Service behavior for shuttingdown?, shutdown? #1042

Merged
merged 1 commit into from
Mar 11, 2024

Conversation

bensheldon
Copy link
Contributor

@bensheldon bensheldon commented Mar 1, 2024

Fixes #1041

  • shuttingdown? now uses isShutdown && !isTerminated
  • shutdown? now accurately represents when all tasks have terminated
  • kill now waits for all tasks to terminate (it waits indefinitely; I'm not sure if it's possible for that to hang on Java) Extracted #kill change to ThreadPoolExecutor kill will wait_for_termination in JRuby #1044

@bensheldon bensheldon force-pushed the java-shuttingdown branch 4 times, most recently from a7d609b to 6f64bb2 Compare March 1, 2024 07:24
@bensheldon bensheldon force-pushed the java-shuttingdown branch 2 times, most recently from 93195e0 to e41c843 Compare March 1, 2024 15:48
@bensheldon
Copy link
Contributor Author

  1. Something in here is causing a deadlock on JRuby in the TimerSet tests (I'm guessing it's the wait_for_termination)
  2. We need a maximum Actions workflow timeout of not 6 hours.

@eregon
Copy link
Collaborator

eregon commented Mar 11, 2024

Could you make a PR to add timeouts of 10 minutes (which looks enough from looking at previous runs) for all CI workflows?

@eregon
Copy link
Collaborator

eregon commented Mar 11, 2024

Can you reproduce the issue locally with TimerSet on JRuby?
I guess it might be related to that kill change.
So it might be worth to extract that change to a separate PR.
In general I wonder if it's really worth the trouble & complexity of having different implementations for JRuby when there is already a pure-Ruby implementation of some concurrent-ruby class.

Co-authored-by: Benoit Daloze <eregontp@gmail.com>
@bensheldon bensheldon changed the title Align Java Executor Service behavior for shuttingdown? shutdown? and kill Align Java Executor Service behavior for shuttingdown? shutdown? Mar 11, 2024
@bensheldon bensheldon changed the title Align Java Executor Service behavior for shuttingdown? shutdown? Align Java Executor Service behavior for shuttingdown?, shutdown? Mar 11, 2024
@bensheldon
Copy link
Contributor Author

@eregon Thank you for the help! 🙇🏻

In general I wonder if it's really worth the trouble & complexity of having different implementations for JRuby when there is already a pure-Ruby implementation of some concurrent-ruby class.

I feel mixed. A lot of Concurrent Ruby is inspired by Java objects, so it seems like it would be better (simpler? more performant? less potential for bikeshedding or 2nd-systeming them?) to lightly wrap them rather than use a Ruby implementation. I have similar vague feelings about Native C Extensions too; which to chit-chat also makes me wonder if YJIT would make that less worthwhile.

@eregon eregon merged commit 8b9b0da into ruby-concurrency:master Mar 11, 2024
13 checks passed
@eregon
Copy link
Collaborator

eregon commented Mar 11, 2024

I have similar vague feelings about Native C Extensions too; which to chit-chat also makes me wonder if YJIT would make that less worthwhile.

The concurrent-ruby C ext is only for atomic variables.
Those are likely quite a bit faster than using a Mutex.
There are some benchmarks for this: e.g. https://github.com/ruby-concurrency/concurrent-ruby/blob/master/examples/benchmark_atomic.rb

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ThreadPoolExecutor#shutdown? inconsistency in JRuby and C Ruby
2 participants