Please review pull request #620: (#3581) Stop forking around: Add fork helper method opened by (kelseyhightower)
Description:
Without this patch we are forking all over the place. All this forking
around is problematic when it comes to things like ActiveRecord and
database connections. It turns out that forking has a nasty side effect
of aggressively closing database connections and spewing errors all over
the place.
This patch introduces a new Puppet::Util#safe_posix_fork method that
does forking the right way (closing file descriptors, and resetting
stdin, stdout, and stderr). Tagmail, Puppet kick, andPuppet::Util#execute_posix have been updated to make use of this new
functionality.
In the future, Puppet::Util#safe_posix_fork should be used in-place of
direct calls to Kernel#fork.
This patch includes related spec tests.
- Opened: Tue Apr 03 18:23:44 UTC 2012
- Based on: puppetlabs:2.7.x (d88f3e7c387eff270101604f2fec2e086f88ebdd)
- Requested merge: kelseyhightower:kelseyhightower/ticket/2.7.x/3581_fork_that (1b810b1b8a5d6ab7f6fd587ee6fa928ed684930a)
Diff follows:
diff --git a/lib/puppet/application/kick.rb b/lib/puppet/application/kick.rb
index 0b6ddc9..5d5ed62 100644
--- a/lib/puppet/application/kick.rb
+++ b/lib/puppet/application/kick.rb
@@ -201,7 +201,7 @@ def main
# do, then do the next host.
if @children.length < options[:parallel] and ! todo.empty?
host = todo.shift
- pid = fork do
+ pid = safe_posix_fork do
run_for_host(host)
end
@children[pid] = host
diff --git a/lib/puppet/reports/tagmail.rb b/lib/puppet/reports/tagmail.rb
index c37341e..0a3b0de 100644
--- a/lib/puppet/reports/tagmail.rb
+++ b/lib/puppet/reports/tagmail.rb
@@ -123,7 +123,7 @@ def process
# Send the email reports.
def send(reports)
- pid = fork do
+ pid = Puppet::Util.safe_posix_fork do
if Puppet[:smtpserver] != "none"
begin
Net::SMTP.start(Puppet[:smtpserver]) do |smtp|
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 8d5968f..7b00df2 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -292,7 +292,7 @@ def execfail(command, exception)
end
def execute_posix(command, arguments, stdin, stdout, stderr)
- child_pid = Kernel.fork do
+ child_pid = safe_posix_fork(stdin, stdout, stderr) do
# We can't just call Array(command), and rely on it returning
# things like ['foo'], when passed ['foo'], because
# Array(command) will call command.to_a internally, which when
@@ -301,12 +301,6 @@ def execute_posix(command, arguments, stdin, stdout, stderr)
command = [command].flatten
Process.setsid
begin
- $stdin.reopen(stdin)
- $stdout.reopen(stdout)
- $stderr.reopen(stderr)
-
- 3.upto(256){|fd| IO::new(fd).close rescue nil}
-
Puppet::Util::SUIDManager.change_privileges(arguments[:uid], arguments[:gid], true)
ENV['LANG'] = ENV['LC_ALL'] = ENV['LC_MESSAGES'] = ENV['LANGUAGE'] = 'C'
@@ -320,6 +314,20 @@ def execute_posix(command, arguments, stdin, stdout, stderr)
end
module_function :execute_posix
+ def safe_posix_fork(stdin=$stdin, stdout=$stdout, stderr=$stderr, &block)
+ child_pid = Kernel.fork do
+ $stdin.reopen(stdin)
+ $stdout.reopen(stdout)
+ $stderr.reopen(stderr)
+
+ 3.upto(256){|fd| IO::new(fd).close rescue nil}
+
+ block.call if block
+ end
+ child_pid
+ end
+ module_function :safe_posix_fork
+
def execute_windows(command, arguments, stdin, stdout, stderr)
command = command.map do |part|
part.include?(' ') ? %Q["#{part.gsub(/"/, '\"')}"] : part
diff --git a/spec/unit/application/kick_spec.rb b/spec/unit/application/kick_spec.rb
index 361aafb..b7c68ac 100755
--- a/spec/unit/application/kick_spec.rb
+++ b/spec/unit/application/kick_spec.rb
@@ -239,14 +239,14 @@
@kick.hosts = ['host1', 'host2', 'host3']
Process.stubs(:wait).returns(1).then.returns(2).then.returns(3).then.raises(Errno::ECHILD)
- @kick.expects(:fork).times(3).returns(1).then.returns(2).then.returns(3)
+ @kick.expects(:safe_posix_fork).times(3).returns(1).then.returns(2).then.returns(3)
expect { @kick.main }.to raise_error SystemExit
end
it "should delegate to run_for_host per host" do
@kick.hosts = ['host1', 'host2']
- @kick.stubs(:fork).returns(1).yields
+ @kick.stubs(:safe_posix_fork).returns(1).yields
Process.stubs(:wait).returns(1).then.raises(Errno::ECHILD)
@kick.expects(:run_for_host).times(2)
diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb
index 37542fe..35485b7 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -223,15 +223,6 @@ def stub_process_wait(exitstatus)
Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr)
end
- it "should close all open file descriptors except stdin/stdout/stderr" do
- # This is ugly, but I can't really think of a better way to do it without
- # letting it actually close fds, which seems risky
- (0..2).each {|n| IO.expects(:new).with(n).never}
- (3..256).each {|n| IO.expects(:new).with(n).returns mock('io', :close) }
-
- Puppet::Util.execute_posix('test command', {}, @stdin, @stdout, @stderr)
- end
-
it "should permanently change to the correct user and group if specified" do
Puppet::Util::SUIDManager.expects(:change_group).with(55, true)
Puppet::Util::SUIDManager.expects(:change_user).with(50, true)
@@ -487,6 +478,38 @@ def stub_process_wait(exitstatus)
}.not_to raise_error
end
end
+
+ describe "safe_posix_fork" do
+ before :each do
+ # Most of the things this method does are bad to do during specs. :/
+ Kernel.stubs(:fork).returns(pid).yields
+
+ $stdin.stubs(:reopen)
+ $stdout.stubs(:reopen)
+ $stderr.stubs(:reopen)
+ end
+
+ it "should close all open file descriptors except stdin/stdout/stderr" do
+ # This is ugly, but I can't really think of a better way to do it without
+ # letting it actually close fds, which seems risky
+ (0..2).each {|n| IO.expects(:new).with(n).never}
+ (3..256).each {|n| IO.expects(:new).with(n).returns mock('io', :close) }
+
+ Puppet::Util.safe_posix_fork
+ end
+
+ it "should fork a child process to execute the block" do
+ Kernel.expects(:fork).returns(pid).yields
+
+ Puppet::Util.safe_posix_fork do
+ message = "Fork this!"
+ end
+ end
+
+ it "should return the pid of the child process" do
+ Puppet::Util.safe_posix_fork.should == pid
+ end
+ end
end
describe "#execpipe" do
-- 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.
