Skip to content

Commit

Permalink
Do not hang in communicate if spawned process forks
Browse files Browse the repository at this point in the history
Currently we only exit the `communicate` loop when we get an EOF from
stdout and stderr. If the child process forks before exiting, the fork
will inherit those file descriptors and `communicate` will hang until
the child exits. I believe it's preferable to drain stdout/stderr after
the process exits and then break out of the `communicate` loop.

Note that Open3 similarly hangs waiting for EOF. There is a risk that
existing software depends on this behavior and that output could be
non-deterministic if the forked grandchild is writing to stdout/stderr.
  • Loading branch information
andrew-stripe committed Nov 7, 2017
1 parent 7993e0e commit cb14315
Show file tree
Hide file tree
Showing 3 changed files with 22 additions and 8 deletions.
18 changes: 11 additions & 7 deletions lib/subprocess.rb
Expand Up @@ -414,13 +414,17 @@ def communicate(input=nil, timeout_s=nil)
self.class.catching_sigchld(pid) do |global_read, self_read|
wait_r = [@stdout, @stderr, self_read, global_read].compact
wait_w = [input && @stdin].compact
loop do
# If the process has exited and we're not waiting to read anything
# other than the self pipe, then we're done.
break if poll && wait_r == [self_read, global_read]

ready_r, ready_w = select_until(wait_r, wait_w, [], timeout_at)
raise CommunicateTimeout.new(@command, stdout, stderr) if ready_r.nil?
done = false
while !done
# If the process has exited, we want to drain any remaining output before returning
if poll
ready_r = wait_r - [self_read, global_read]
ready_w = []
done = true
else
ready_r, ready_w = select_until(wait_r, wait_w, [], timeout_at)
raise CommunicateTimeout.new(@command, stdout, stderr) if ready_r.nil?
end

if ready_r.include?(@stdout)
if drain_fd(@stdout, stdout)
Expand Down
2 changes: 1 addition & 1 deletion lib/subprocess/version.rb
@@ -1,3 +1,3 @@
module Subprocess
VERSION = '1.5.2'
VERSION = '1.5.3'
end
10 changes: 10 additions & 0 deletions test/test_subprocess.rb
Expand Up @@ -294,6 +294,16 @@ def call_multiwrite_script(&block)
end
end

it 'does not deadlock when communicating with a process that forks' do
script = <<EOF
echo "fork"
(echo "me" && sleep 10) &
EOF
p = Subprocess::Process.new(['bash', '-c', script], stdout: Subprocess::PIPE)
stdout, _stderr = p.communicate(nil, 5)
stdout.must_include("fork\n")
end

it 'should not raise an error when the process closes stdin before we finish writing' do
script = File.join(File.dirname(__FILE__), 'bin', 'closer.rb')
Subprocess.check_call(['bash', '-c', '<&-; echo -n "foo"; sleep 1'], :stdin => Subprocess::PIPE,
Expand Down

0 comments on commit cb14315

Please sign in to comment.