This patch incorporates Ricky Zhou's idea for establishing a default SIGPIPE
trap of IGNORE in child, which protects really dumb daemons from the effects of
writing to a closed stdout. As even moderately smart daemons ought not do
this, these may be a reasonable fraction of the cases.
It also includes a sketch of the test for the patch, which is marked pending
as there is no good way to implement it as a portable automated test; you
either need a small standalone C program, a hacked ruby interpreter, or other
shenanigans to run it.
Also, tightened tests (~40 seconds instead of ~200).
---
lib/puppet/util.rb | 7 ++++
spec/unit/exec_test_helper | 6 ++-
spec/unit/util.rb | 86 ++++++++++++++++++++-----------------------
3 files changed, 51 insertions(+), 48 deletions(-)
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index a37a917..158a507 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -322,6 +322,13 @@ module Util
$stdout.reopen('/dev/null', 'w')
$stderr.reopen('/dev/null', 'w')
else
+ # Ignore SIGPIPE. This allows daemons which do not
+ # properly close stdout/stderr to receive EPIPE on
+ # attempting to write to the pipe instead of being
+ # terminated with a SIGPIPE. See ticket #1563,
+ # comment 7 and ticket #3013 for details.
+ trap(:PIPE, 'SIG_IGN')
+
$stdout.reopen(output_write)
if arguments[:combine]
$stderr.reopen(output_write)
diff --git a/spec/unit/exec_test_helper b/spec/unit/exec_test_helper
index ac536fe..f00814d 100755
--- a/spec/unit/exec_test_helper
+++ b/spec/unit/exec_test_helper
@@ -1,6 +1,7 @@
#!/usr/bin/env ruby
require 'optparse'
+$trap_and_relay = []
options = OptionParser.new
options.on("--kill-me-withi SIGNAL",/USR[12]/) { |signal| $kill_me = signal }
options.on("--called-by PID",Integer) { |parent| $pid = parent }
@@ -13,6 +14,7 @@ options.on("--flush") {
$flush = true }
options.on("--fork-badly") { $fork_badly = true }
options.on("--write-in-fork") {
$write_in_fork = true }
options.on("--signal-exit") { $signal_exit = true }
+options.on("--trap-and-relay SIGNAL",/PIPE/) { |val|
$trap_and_relay |= [val] }
rest = options.parse(ARGV)
unless rest.empty?
@@ -34,8 +36,8 @@ end
unless $fork_badly and fork
$exit_signal = :USR2 # Something unexpected happened
- trap(:PIPE) { $exit_signal = :PIPE }
- at_exit { Process.kill $exit_signal,$pid} if $signal_exit
+ $trap_and_relay.each { |signal| trap(signal) { $exit_signal = signal } }
+ at_exit { Process.kill $exit_signal,$pid } if $signal_exit
sleep $delay_close || 0
puts 'Thanks for waiting' if $write_in_fork
$stdout.close
diff --git a/spec/unit/util.rb b/spec/unit/util.rb
index b178e19..79cc9d8 100755
--- a/spec/unit/util.rb
+++ b/spec/unit/util.rb
@@ -116,60 +116,54 @@ describe Puppet::Util do
end
it "should not mind (hang, crash, lose output) if the
other end closes the pipe before completing" do
n = 5
- task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--repeat',n,'--delay-exit',5]
- timeout(10) { Puppet::Util.execute(task).should
== "#{$PID}\n"*n }
+ task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--repeat',n,'--delay-exit',2]
+ timeout(5) { Puppet::Util.execute(task).should ==
"#{$PID}\n"*n }
end
it "should not hang if the child process terminates
but does not close the pipe" do
# Ticket #1563 comment #7
n = 5
- task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',50]
+ task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',100]
timeout(20) { Puppet::Util.execute(task).should
== "#{$PID}\n"*n }
end
describe "when the child process passes the pipe to a
grandchild and terminates without closing the pipe or waiting for the
grandchild" do
- it "should not cause the grandchild child process
to receive a SIGPIPE if the grandchild subsequently ignores the pipe"
do
- # Ticket #1563 comment #7, null hypothesis
for Ticket #3013 comment #18
- n = 5
- task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',30,'--signal-exit']
- # Note that the trapping setup/teardown
appaently can NOT be done in a before block
- signal = nil
- old_usr1_trap = trap(:USR1) { signal = :USR1
} # They exited normally
- old_usr2_trap = trap(:USR2) { signal = :USR2
} # They died
- old_pipe_trap = trap(:PIPE) { signal = :PIPE
} # They got a SIGPIPE
- timeout(50) {
- Puppet::Util.execute(task).should == "#{$PID}\n"*n
- `sleep 0` until signal # Process sleep,
not thread sleep
- }
- trap(:USR1,old_usr1_trap)
- trap(:USR2,old_usr2_trap)
- trap(:PIPE,old_pipe_trap)
- signal.should == :USR1
+ [
+ [
+ "the grandchild subsequently ignores the
pipe", # Ticket #1563 comment #7, null hypothesis for Ticket #3013
comment #18
+ ['--signal-exit'],
+ :USR1
+ ],[
+ "the grandchild subsequently writes to
the pipe without establishing a SIGPIPE trap", # Ticket #3013 comment
#18 & #22ff
+ ['--write-in-fork','--signal-exit'],
+ :USR1,
+ "This can be demonstrated (see the
ticket) but not in a portable way because ruby _always_ traps SIGPIPE
even if the user program dosn't."
+ ],[
+ "the grandchild subsequently writes to
the pipe after establishing a SIGPIPE trap", # Ticket #3013 comment
#28 & #29
+
['--write-in-fork','--signal-exit','--trap-and-relay',:PIPE],
+ :PIPE
+ ]
+ ].each { |desc,flags,expected,pending_reason|
+ it "should #{(expected == :USR1) ? 'not ' :
''}cause the grandchild child process to receive a SIGPIPE if #{desc}"
do
+ pending pending_reason if pending_reason
+ n = 5
+ task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',2,*flags]
+ # Note that the trapping setup/teardown
appaently can NOT be done in a before block
+ signal = nil
+ old_usr1_trap = trap(:USR1) { signal =
:USR1 } # They exited normally
+ old_usr2_trap = trap(:USR2) { signal =
:USR2 } # They died
+ old_pipe_trap = trap(:PIPE) { signal =
:PIPE } # They got a SIGPIPE
+ timeout(50) {
+ Puppet::Util.execute(task).should ==
"#{$PID}\n"*n
+ `sleep 0` until signal # Process
sleep, not thread sleep
+ }
+ trap(:USR1,old_usr1_trap)
+ trap(:USR2,old_usr2_trap)
+ trap(:PIPE,old_pipe_trap)
+ signal.should == expected
+ end
+ }
+ it "should be able to deal with the command-line
'pseudo-daemon&' idiom (but not retain the output)" do
+ Puppet::Util.execute(['/bin/sh','-c','(sleep
5; echo foo)&']).should == ''
end
- it "should not cause the grandchild child process
to receive a SIGPIPE if the grandchild subsequently writes to the
pipe" do
- # Ticket #3013 comment #18
- pending %q{
- This situation is presently only a
speculation and may in any case exceed
- what we should expect the code to do; see
note in the implementation for a
- suggested way to handle it if needed.
- }
- n = 5
- task =
[File.dirname(__FILE__)+'/exec_test_helper','--called-by',$PID,'--fork-badly','--repeat',n,'--delay-close',30,'--write-in-fork','--signal-exit']
- # Note that the trapping setup/teardown
apparently can NOT be done in a before block
- signal = nil
- old_usr1_trap = trap(:USR1) { signal = :USR1
} # They exited normally
- old_usr2_trap = trap(:USR2) { signal = :USR2
} # They died
- old_pipe_trap = trap(:PIPE) { signal = :PIPE
} # They got a SIGPIPE
- timeout(50) {
- Puppet::Util.execute(task).should == "#{$PID}\n"*n
- `sleep 0` until signal # Process sleep,
not thread sleep
- }
- trap(:USR1,old_usr1_trap)
- trap(:USR2,old_usr2_trap)
- trap(:PIPE,old_pipe_trap)
- signal.should == :USR1
- end
- it "should be able to deal with the command-line
'pseudo-daemon&' idiom (but not retain the output)" do
- Puppet::Util.execute(['/bin/sh','-c','(sleep
5; echo foo)&']).should == ''
- end
end
end
}
--
1.6.4
--
You received this message because you are subscribed to the Google Groups
"Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to
[email protected].
For more options, visit this group at
http://groups.google.com/group/puppet-dev?hl=en.
