Please review pull request #520: (#11740) Wait on the handle from the PROCESS_INFORMATION structure opened by (joshcooper)
Description:
Previously, the Puppet::Util.execute method was using waitpid2 to
wait for the child process to exit and retrieve its exit status. On
Windows, this method (as implemented by the win32-process gem) opens a
handle to the process with the given pid, and then callsWaitForSingleObject and GetExitCodeProcess on the process
handle. However, the pid-to-handle lookup will raise an exception if
the child process has exited. As a result there was a race condition
whereby puppet could sometimes fail to retrieve the exit status of
child processes.
The normal way of getting the exit code for a child process on Windows
is to use the child process handle contained in thePROCESS_INFORMATION structure returned by CreateProcess. This
works regardless of whether the child process is currently executing
or not.
This commit reworks the Puppet::Util.execute_windows method to wait
on the child process handle contained in the process information
structure. This requires that we pass the :close_handles => false
option to the win32-process gem so that it doesn't close the handles.
This commit also re-enables tests that were previously being skipped
due to this bug.
- Opened: Tue Feb 21 00:40:21 UTC 2012
- Based on: puppetlabs:2.7.x (c5cf798ca1d023352efb099fc1817a6948bfe8ce)
- Requested merge: joshcooper:ticket/2.7.x/11740-handle-is-invalid (c5d382563e6701261dce188257e49aac5e02fcf6)
Diff follows:
diff --git a/acceptance/tests/resource/exec/should_run_command.rb b/acceptance/tests/resource/exec/should_run_command.rb
index 4a76cc4..eff86cc 100644
--- a/acceptance/tests/resource/exec/should_run_command.rb
+++ b/acceptance/tests/resource/exec/should_run_command.rb
@@ -15,11 +15,6 @@ def after(agent, touched)
end
agents.each do |agent|
- if agent['platform'].include?('windows')
- Log.warn('Pending due to #11740')
- next
- end
-
touched = before(agent)
apply_manifest_on(agent, "exec {'test': command=>'#{agent.touch(touched)}'}") do
fail_test "didn't seem to run the command" unless
diff --git a/lib/puppet/util.rb b/lib/puppet/util.rb
index 5203fb0..b4c21c2 100644
--- a/lib/puppet/util.rb
+++ b/lib/puppet/util.rb
@@ -311,8 +311,7 @@ def execute_windows(command, arguments, stdin, stdout, stderr)
part.include?(' ') ? %Q["#{part.gsub(/"/, '\"')}"] : part
end.join(" ") if command.is_a?(Array)
- process_info = Process.create( :command_line => command, :startup_info => {:stdin => stdin, :stdout => stdout, :stderr => stderr} )
- process_info.process_id
+ Puppet::Util::Windows::Process.execute(command, arguments, stdin, stdout, stderr)
end
module_function :execute_windows
@@ -349,13 +348,13 @@ def execute(command, arguments = {:failonfail => true, :combine => true})
child_pid = execute_posix(*exec_args)
exit_status = Process.waitpid2(child_pid).last.exitstatus
elsif Puppet.features.microsoft_windows?
- child_pid = execute_windows(*exec_args)
- exit_status = Process.waitpid2(child_pid).last
- # $CHILD_STATUS is not set when calling win32/process Process.create
- # and since it's read-only, we can't set it. But we can execute a
- # a shell that simply returns the desired exit status, which has the
- # desired effect.
- %x{#{ENV['COMSPEC']} /c exit #{exit_status}}
+ process_info = execute_windows(*exec_args)
+ begin
+ exit_status = Puppet::Util::Windows::Process.wait_process(process_info.process_handle)
+ ensure
+ Process.CloseHandle(process_info.process_handle)
+ Process.CloseHandle(process_info.thread_handle)
+ end
end
[stdin, stdout, stderr].each {|io| io.close rescue nil}
diff --git a/lib/puppet/util/windows.rb b/lib/puppet/util/windows.rb
index a5ff82a..086e08c 100644
--- a/lib/puppet/util/windows.rb
+++ b/lib/puppet/util/windows.rb
@@ -2,4 +2,5 @@ module Puppet::Util::Windows
require 'puppet/util/windows/error'
require 'puppet/util/windows/security'
require 'puppet/util/windows/user'
+ require 'puppet/util/windows/process'
end
diff --git a/lib/puppet/util/windows/process.rb b/lib/puppet/util/windows/process.rb
new file mode 100644
index 0000000..782a381
--- /dev/null
+++ b/lib/puppet/util/windows/process.rb
@@ -0,0 +1,31 @@
+require 'puppet/util/windows'
+
+module Puppet::Util::Windows::Process
+ extend Windows::Process
+ extend Windows::Handle
+ extend Windows::Synchronize
+
+ def execute(command, arguments, stdin, stdout, stderr)
+ Process.create( :command_line => command, :startup_info => {:stdin => stdin, :stdout => stdout, :stderr => stderr}, :close_handles => false )
+ end
+ module_function :execute
+
+ def wait_process(handle)
+ WaitForSingleObject(handle, Windows::Synchronize::INFINITE)
+
+ exit_status = [0].pack('L')
+ unless GetExitCodeProcess(handle, exit_status)
+ raise Puppet::Util::Windows::Error.new("Failed to get child process exit code")
+ end
+ exit_status = exit_status.unpack('L').first
+
+ # $CHILD_STATUS is not set when calling win32/process Process.create
+ # and since it's read-only, we can't set it. But we can execute a
+ # a shell that simply returns the desired exit status, which has the
+ # desired effect.
+ %x{#{ENV['COMSPEC']} /c exit #{exit_status}}
+
+ exit_status
+ end
+ module_function :wait_process
+end
diff --git a/spec/unit/util/execution_stub_spec.rb b/spec/unit/util/execution_stub_spec.rb
index b66f88f..57305e3 100755
--- a/spec/unit/util/execution_stub_spec.rb
+++ b/spec/unit/util/execution_stub_spec.rb
@@ -16,8 +16,7 @@
Puppet::Util::ExecutionStub.current_value.should == nil
end
- # fails on windows, see #11740
- it "should restore normal execution after 'reset' is called", :fails_on_windows => true do
+ it "should restore normal execution after 'reset' is called" do
# Note: "true" exists at different paths in different OSes
if Puppet.features.microsoft_windows?
true_command = [Puppet::Util.which('cmd.exe').tr('/', '\\'), '/c', 'exit 0']
diff --git a/spec/unit/util_spec.rb b/spec/unit/util_spec.rb
index 35358d6..980ec84 100755
--- a/spec/unit/util_spec.rb
+++ b/spec/unit/util_spec.rb
@@ -5,12 +5,6 @@
describe Puppet::Util do
include PuppetSpec::Files
- def process_status(exitstatus)
- return exitstatus if Puppet.features.microsoft_windows?
-
- stub('child_status', :exitstatus => exitstatus)
- end
-
describe "#absolute_path?" do
it "should default to the platform of the local system" do
Puppet.features.stubs(:posix?).returns(true)
@@ -159,8 +153,21 @@ def process_status(exitstatus)
describe "execution methods" do
let(:pid) { 5501 }
+ let(:process_handle) { 0xDEADBEEF }
+ let(:thread_handle) { 0xCAFEBEEF }
+ let(:proc_info_stub) { stub 'processinfo', :process_handle => process_handle, :thread_handle => thread_handle, :process_id => pid}
let(:null_file) { Puppet.features.microsoft_windows? ? 'NUL' : '/dev/null' }
+ def stub_process_wait(exitstatus)
+ if Puppet.features.microsoft_windows?
+ Puppet::Util::Windows::Process.stubs(:wait_process).with(process_handle).returns(exitstatus)
+ Process.stubs(:CloseHandle).with(process_handle)
+ Process.stubs(:CloseHandle).with(thread_handle)
+ else
+ Process.stubs(:waitpid2).with(pid).returns([pid, stub('child_status', :exitstatus => exitstatus)])
+ end
+ end
+
describe "#execute_posix" do
before :each do
# Most of the things this method does are bad to do during specs. :/
@@ -233,12 +240,10 @@ def process_status(exitstatus)
end
end
- describe "#execute_windows" do
- let(:proc_info_stub) { stub 'processinfo', :process_id => pid }
-
+ describe "#execute_windows", :if => Puppet.features.microsoft_windows? do
before :each do
Process.stubs(:create).returns(proc_info_stub)
- Process.stubs(:waitpid2).with(pid).returns([pid, process_status(0)])
+ stub_process_wait(0)
@stdin = File.open(null_file, 'r')
@stdout = Tempfile.new('stdout')
@@ -248,14 +253,15 @@ def process_status(exitstatus)
it "should create a new process for the command" do
Process.expects(:create).with(
:command_line => "test command",
- :startup_info => {:stdin => @stdin, :stdout => @stdout, :stderr => @stderr}
+ :startup_info => {:stdin => @stdin, :stdout => @stdout, :stderr => @stderr},
+ :close_handles => false
).returns(proc_info_stub)
Puppet::Util.execute_windows('test command', {}, @stdin, @stdout, @stderr)
end
- it "should return the pid of the child process" do
- Puppet::Util.execute_windows('test command', {}, @stdin, @stdout, @stderr).should == pid
+ it "should return the process info of the child process" do
+ Puppet::Util.execute_windows('test command', {}, @stdin, @stdout, @stderr).should == proc_info_stub
end
it "should quote arguments containing spaces if command is specified as an array" do
@@ -269,7 +275,7 @@ def process_status(exitstatus)
describe "#execute" do
before :each do
- Process.stubs(:waitpid2).with(pid).returns([pid, process_status(0)])
+ stub_process_wait(0)
end
describe "when an execution stub is specified" do
@@ -294,6 +300,7 @@ def process_status(exitstatus)
describe "when setting up input and output files" do
include PuppetSpec::Files
let(:executor) { Puppet.features.microsoft_windows? ? 'execute_windows' : 'execute_posix' }
+ let(:rval) { Puppet.features.microsoft_windows? ? proc_info_stub : pid }
before :each do
Puppet::Util.stubs(:wait_for_output)
@@ -305,7 +312,7 @@ def process_status(exitstatus)
Puppet::Util.expects(executor).with do |_,_,stdin,_,_|
stdin.path == input
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command', :stdinfile => input)
end
@@ -313,7 +320,7 @@ def process_status(exitstatus)
it "should set stdin to the null file if not specified" do
Puppet::Util.expects(executor).with do |_,_,stdin,_,_|
stdin.path == null_file
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command')
end
@@ -322,7 +329,7 @@ def process_status(exitstatus)
it "should set stdout and stderr to the null file" do
Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == null_file and stderr.path == null_file
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command', :squelch => true)
end
@@ -335,7 +342,7 @@ def process_status(exitstatus)
Puppet::Util.expects(executor).with do |_,_,_,stdout,_|
stdout.path == outfile.path
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command', :squelch => false)
end
@@ -346,7 +353,7 @@ def process_status(exitstatus)
Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == outfile.path
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command', :squelch => false, :combine => true)
end
@@ -357,28 +364,40 @@ def process_status(exitstatus)
Puppet::Util.expects(executor).with do |_,_,_,stdout,stderr|
stdout.path == outfile.path and stderr.path == null_file
- end.returns(pid)
+ end.returns(rval)
Puppet::Util.execute('test command', :squelch => false, :combine => false)
end
end
end
+
+ describe "on Windows", :if => Puppet.features.microsoft_windows? do
+ it "should always close the process and thread handles" do
+ Puppet::Util.stubs(:execute_windows).returns(proc_info_stub)
+
+ Puppet::Util::Windows::Process.expects(:wait_process).with(process_handle).raises('whatever')
+ Process.expects(:CloseHandle).with(thread_handle)
+ Process.expects(:CloseHandle).with(process_handle)
+
+ expect { Puppet::Util.execute('test command') }.should raise_error(RuntimeError)
+ end
+ end
end
describe "after execution" do
- let(:executor) { Puppet.features.microsoft_windows? ? 'execute_windows' : 'execute_posix' }
-
before :each do
- Process.stubs(:waitpid2).with(pid).returns([pid, process_status(0)])
+ stub_process_wait(0)
- Puppet::Util.stubs(executor).returns(pid)
+ if Puppet.features.microsoft_windows?
+ Puppet::Util.stubs(:execute_windows).returns(proc_info_stub)
+ else
+ Puppet::Util.stubs(:execute_posix).returns(pid)
+ end
end
it "should wait for the child process to exit" do
Puppet::Util.stubs(:wait_for_output)
- Process.expects(:waitpid2).with(pid).returns([pid, process_status(0)])
-
Puppet::Util.execute('test command')
end
@@ -423,7 +442,7 @@ def process_status(exitstatus)
end
it "should raise an error if failonfail is true and the child failed" do
- Process.expects(:waitpid2).with(pid).returns([pid, process_status(1)])
+ stub_process_wait(1)
expect {
Puppet::Util.execute('fail command', :failonfail => true)
@@ -431,7 +450,7 @@ def process_status(exitstatus)
end
it "should not raise an error if failonfail is false and the child failed" do
- Process.expects(:waitpid2).with(pid).returns([pid, process_status(1)])
+ stub_process_wait(1)
expect {
Puppet::Util.execute('fail command', :failonfail => false)
@@ -439,8 +458,6 @@ def process_status(exitstatus)
end
it "should not raise an error if failonfail is true and the child succeeded" do
- Process.expects(:waitpid2).with(pid).returns([pid, process_status(0)])
-
expect {
Puppet::Util.execute('fail command', :failonfail => true)
}.not_to raise_error
-- 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.
