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

Fix for spawning subprocesses with fork_worker option #2267

Merged
merged 2 commits into from May 20, 2020

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented May 15, 2020

Description

This fixes a bug in fork_worker mode (#2099) where an application that spawns subprocesses can sometimes get nil instead of an expected Process::Status object when waiting on the child (see #2099 (comment)). test_fork_worker_spawn demonstrates the regression and currently fails on master:

def test_fork_worker_spawn
cli_server '', config: <<RUBY
workers 1
fork_worker 0
app do |_|
pid = spawn('ls', [:out, :err]=>'/dev/null')
sleep 0.01
exitstatus = Process.detach(pid).value.exitstatus
[200, {}, [exitstatus.to_s]]
end
RUBY
assert_equal '0', read_body(connect)
end

This PR is one of two approaches for a fix- it stores the list of forked worker pids and the SIGCHLD handler calls non-blocking Process.wait on each of them, removing the pids that have exited. This is similar to #wait_workers.

Worth noting that this approach results in a wait syscall for every worker for every subprocess spawned by worker 0's application, which might cause some performance impact for applications which spawn lots of subprocesses and/or have a large number of workers. (I haven't benchmarked in detail so I don't know if it really matters or how big an impact it might be.)

An alternative approach (aacd629) avoids a SIGCHLD handler and instead spawns a separate thread for each forked worker that calls blocking Process.wait, to automatically clean up each process as it exits. This is similar to Process.detach- it avoids the extra syscall per worker * subprocess, at the cost of an extra thread per worker. That approach was discouraged in #2099 (comment) but I thought worth mentioning in any case.

Your checklist for this pull request

  • I have reviewed the guidelines for contributing to this repository.
  • I have added an entry to History.md if this PR fixes a bug or adds a feature. If it doesn't need an entry to HISTORY.md, I have added [changelog skip] the pull request title.
  • I have added appropriate tests if this PR fixes a bug or adds a feature.
  • My pull request is 100 lines added/removed or less so that it can be easily reviewed.
  • If this PR doesn't need tests (docs change), I added [ci skip] to the title of the PR.
  • If this closes any issues, I have added "Closes #issue" to the PR description or my commit messages.
  • I have updated the documentation accordingly.
  • All new and existing tests passed, including Rubocop.

@nateberkopec
Copy link
Member

I'm OK with this approach as fork_worker is experimental. If someone runs into a perf issue w/this fix we can fix it at that point. No need to worry about theoretical bottlenecks on a feature that we're not sure is any good yet.

@nateberkopec nateberkopec merged commit 2ded685 into puma:master May 20, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants