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

Create new pgroup when spawning process in chdir #5383

Merged
merged 1 commit into from
Dec 18, 2018

Conversation

davishmcclurg
Copy link
Contributor

Process.spawn is leaving orphaned processes when run inside
Dir.chdir:

>> JRUBY_VERSION
=> "9.2.0.0"
>> Dir.chdir('/tmp') { Process.spawn('cat') }
=> 88255
$ ps xao pid,ppid,pgid,comm | grep -E ' (cat|sh)'
88255 88092 88092 sh
88256 88255 88092 cat
>> Process.kill('TERM', 88255)
=> 1
$ ps xao pid,ppid,pgid,comm | grep -E ' (cat|sh)'
88256     1 88092 cat

I believe the issue is that adding the cd command (for chdir) causes a
second process to start. That process is grouped with the main java one
and doesn't die when its parent (sh) is killed.

This change creates a new process group for the sh command, which
causes the child process to die when the parent is killed.

I ran into this issue using Foreman to manage some JRuby processes.

`Process.spawn` is leaving orphaned processes when run inside
`Dir.chdir`:

```ruby
>> JRUBY_VERSION
=> "9.2.0.0"
>> Dir.chdir('/tmp') { Process.spawn('cat') }
=> 88255
```

```bash
$ ps xao pid,ppid,pgid,comm | grep -E ' (cat|sh)'
88255 88092 88092 sh
88256 88255 88092 cat
```

```ruby
>> Process.kill('TERM', 88255)
=> 1
```

```bash
$ ps xao pid,ppid,pgid,comm | grep -E ' (cat|sh)'
88256     1 88092 cat
```

I believe the issue is that adding the `cd` command (for chdir) causes a
second process to start. That process is grouped with the main java one
and doesn't die when its parent (`sh`) is killed.

This change creates a new process group for the `sh` command, which
causes the child process to die when the parent is killed.

I ran into this issue using Foreman to manage some JRuby processes.
@davishmcclurg
Copy link
Contributor Author

Another option for this is using "cd '" + eargp.chdir_dir + "'; exec " to have the process be replaced after changing directories. I wasn't sure what that would mean for Windows support, though, and I don't have access to a machine for testing.

@kares kares added this to the JRuby 9.2.4.0 milestone Nov 13, 2018
@enebo enebo modified the milestones: JRuby 9.2.4.0, JRuby 9.2.5.0 Nov 13, 2018
@enebo enebo modified the milestones: JRuby 9.2.5.0, JRuby 9.2.6.0 Dec 6, 2018
@headius
Copy link
Member

headius commented Dec 18, 2018

This looks good, and it's a great discovery. Thank you!

@enebo enebo merged commit d968de4 into jruby:master Dec 18, 2018
@davishmcclurg davishmcclurg deleted the process-spawn-chdir-pgroup branch October 8, 2019 16:51
headius added a commit to headius/jruby that referenced this pull request Feb 26, 2021
In jruby#5383 we added logic to create a new process group when doing
chdir in spawn, to avoid orphaning the eventual process spawned
by the intermediate sh. Unfortunately this has a side effect of
hanging on waitpid if the process spawns a long-running daemon.

This patch removes the process group and instead uses exec in the
generated sh script so that the sh process is replaced. The forked
daemon lives on, as it should, but only the main process is
waited on.

Fixes jruby#6579
headius added a commit to headius/jruby that referenced this pull request Feb 26, 2021
In jruby#5383 we added logic to create a new process group when doing
chdir in spawn, to avoid orphaning the eventual process spawned
by the intermediate sh. Unfortunately this has a side effect of
hanging on waitpid if the process spawns a long-running daemon.

This patch removes the process group and instead uses exec in the
generated sh script so that the sh process is replaced. The forked
daemon lives on, as it should, but only the main process is
waited on.

Fixes jruby#6579
headius added a commit to headius/jruby that referenced this pull request Feb 26, 2021
In jruby#5383 we added logic to create a new process group when doing
chdir in spawn, to avoid orphaning the eventual process spawned
by the intermediate sh. Unfortunately this has a side effect of
hanging on waitpid if the process spawns a long-running daemon.

This patch removes the process group and instead uses exec in the
generated sh script so that the sh process is replaced. The forked
daemon lives on, as it should, but only the main process is
waited on.

Fixes jruby#6579
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.

None yet

5 participants