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

Run processes in new process groups, kill process group instead of process #780

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dentarg
Copy link

@dentarg dentarg commented Jun 26, 2021

This is #723 but rebased against master today. Credit to @davishmcclurg for suggesting those changes in the first place.

Some processes are getting orphaned when running Foreman with JRuby.
Creating a new pgroup allows them all to be killed together.

I believe the issue is related to how JRuby handles Dir.chdir by
creating a shell process: sh -c 'cd /chdir/target; ${command}'. That
causes a second process to be created that won't get cleaned up by
killing the parent.

I think this change also addresses #779. It makes Foreman match what Honcho does, and Honcho does not exhibit the problem shown in #779.

Close #779

(Some additional background: There was an earlier PR #528 that did half of what this PR does, #525 was closed after that was merged, but then the PR was reverted after a few days, could not find the reason for that).

Some processes are getting orphaned when running Foreman with JRuby.
Creating a new pgroup allows them all to be killed together.

I believe the issue is related to how JRuby handles `Dir.chdir` by
creating a shell process: `sh -c 'cd /chdir/target; ${command}'`. That
causes a second process to be created that won't get cleaned up by
killing the parent.
@domenkozar
Copy link

cc @ddollar this is my only day to day pain with foreman.

Would love getting this get merged.

@sjjbirch
Copy link

sjjbirch commented Aug 9, 2022

@domenkozar I think it's time to fork and release a new gem. The issue this PR addresses drives me nuts as well and it's getting close to a year for a branch with no conflicts.

@domenkozar
Copy link

I maintain way too many OSS projects, but if you decide to go for it, I'm happy to test.

@baelter
Copy link

baelter commented Oct 28, 2022

Ping @ddollar

@dentarg
Copy link
Author

dentarg commented Oct 28, 2022

foreman forked: https://github.com/spinels/overman, https://rubygems.org/gems/overman

The published version is current master (a5f9b78) + this PR. The executable is called overman so you can have both gems installed at the same time. You can also uninstall foreman and make foreman an alias to overman.

Please try it!

@oleg-vinted
Copy link

This PR makes it better, but it's not enough, see spinels#2.

@oleg-vinted
Copy link

For what it's worth, neither goreman, forego, nor overmind exhibit this behavior.

@msakrejda
Copy link

For what it's worth, I was running into orphaned children in some situations after a Ctrl+C on foreman, and switching to overman resolved that.

msakrejda added a commit to pganalyze/foreman that referenced this pull request Jul 17, 2024
@msakrejda
Copy link

Scratch that: it resolved the issue on my system, but some colleagues ran into weird behavior with some spawned processes (specifically the Vite dev server).

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.

Foreman does not terminate child process
6 participants