Skip to content

Kill replaced worker processes when interrupted#370

Open
wintonpc wants to merge 5 commits intogrosser:masterfrom
wintonpc:kill-isolated
Open

Kill replaced worker processes when interrupted#370
wintonpc wants to merge 5 commits intogrosser:masterfrom
wintonpc:kill-isolated

Conversation

@wintonpc
Copy link
Copy Markdown

@wintonpc wintonpc commented Apr 13, 2026

Related issue: #369

I preserved the existing contract as much as I could, but there's a non-zero risk of breaking code that uses the gem. Given the niche use case and easy work around, I'm not sure the reward outweighs the risk. (Although since parallel just bumped to 2.x, it may be close to an appropriate time for a potentially breaking change.)

Comment thread lib/parallel.rb Outdated
kill_thread = Thread.new do
pids = options[:mutex].synchronize do
@to_be_killed.flatten(1).map(&:pid)
# FUTURE: stop JobFactory from spawning new workers
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this properly would involve a new Mutex#synchronize around both the "stopped" check and the call to replace_worker, which I'm hesitant to add without understanding the code better.

Comment thread spec/parallel_spec.rb
@@ -0,0 +1,13 @@
# frozen_string_literal: true
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some description here what this is doing/simulating

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added.

Comment thread lib/parallel.rb Outdated
@grosser
Copy link
Copy Markdown
Owner

grosser commented Apr 13, 2026

no super excited for this ... killed process just continuing could also be seen as a bug

@wintonpc
Copy link
Copy Markdown
Author

no super excited for this ... killed process just continuing could also be seen as a bug

No problem. As I mentioned, there's a reasonable workaround. I just wanted to submit a best effort fix along with the bug report.

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.

2 participants