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

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.

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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