Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

(#3581) Stop forking around: Add fork helper method #620

Merged
merged 1 commit into from

4 participants

@kelseyhightower

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, and
Puppet::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.

Kelsey Hightower (#3581) Stop forking around: Add fork helper method
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, and
`Puppet::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.
1b810b1
@stschulte
Collaborator

Does it actually work and ActiveRecord runs correctly? I do not know if calling close is the right thing. And counting from 3 to 256 seems a bit arbitrary to me since a process can hold much more than 256 filehandles. But i stumbled over the following implementation I found here http://cmdrclueless.com/blog/2009/07/06/activerecord-fork-exec-boom/

require 'fcntl'

keepers = [STDIN,STDOUT,STDERR]
ObjectSpace.each_object(IO) do |io|
  if not io.closed? and not keepers.include?(io)
    flags = io.fcntl(Fcntl::F_GETFD, 0)
    io.fcntl(Fnctl::F_SETFD, Fcntl::FD_CLOEXEC | flags)
  end
end

But I dont know the real difference between the closeonexec flag and calling the "ruby close".

@kelseyhightower

@stschulte Thanks for the link, I'm going to update this pull and try something like that.

@daniel-pittman

@stschulte - for the record, yes, that actually works, and is correct. (This is, in fact, what Ruby does when you use popen, or backticks, or any number of similar ways to fork and capture output.)

Using 3 through 256 is pretty much arbitrary, but it is more correct - we want to close all file handles, not just those exposed at the Ruby level as IO objects. The arbitrary part is that, indeed, you can have more than 256 handles, but - Puppet pretty much never should, and MRI has no portable mechanism to obtain the maximum file handle number.

Finally, CLOEXEC is a (recent) Linux-ism, and isn't available on many platforms.

@daniel-pittman

@stahnma - it should be completely irrelevant in terms of the possible file descriptor leak, since that is in a single process, not across fork. The other two reflect the same thing - something is causing AR to leak connections. This isn't that - it would shut down the connection incorrectly instead. Sorry.

@daniel-pittman daniel-pittman merged commit 0b13a37 into from
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 3, 2012
  1. (#3581) Stop forking around: Add fork helper method

    Kelsey Hightower authored
    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, and
    `Puppet::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.
This page is out of date. Refresh to see the latest.
View
2  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
View
2  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|
View
22 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
View
4 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)
View
41 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
Something went wrong with that request. Please try again.