From 91ef5a0084ce56c176b5d312a81a02ffdf383920 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Thu, 11 Jun 2020 19:05:19 +0200 Subject: [PATCH 01/11] Detach bisect process to avoid making zombie process If we do not `waitpid` or `detach` the bisect process become a zombie process. As mentionned in waitpid doc: > As long as a zombie is not removed from the system via a wait, it will consume a slot in the kernel process table, and if this table fills, it will not be possible to create further processes. `detach` is a good idea. From the Ruby doc: > Some operating systems retain the status of terminated child processes until the parent collects that status (normally using some variant of wait()). If the parent never collects this status, the child stays around as a zombie process. Process::detach prevents this by setting up a separate Ruby thread whose sole job is to reap the status of the process pid when it terminates. Use detach only when you do not intend to explicitly wait for the child to terminate. Related: - https://github.com/rspec/rspec-core/pull/2669 - https://andrykonchin.github.io/rails/2019/12/25/deadlock-in-rspec.html --- lib/rspec/core/bisect/fork_runner.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/lib/rspec/core/bisect/fork_runner.rb b/lib/rspec/core/bisect/fork_runner.rb index b772e81af3..c00ab1d98b 100644 --- a/lib/rspec/core/bisect/fork_runner.rb +++ b/lib/rspec/core/bisect/fork_runner.rb @@ -91,9 +91,12 @@ def initialize(runner, channel) end def dispatch_specs(run_descriptor) - fork { run_specs(run_descriptor) } + pid = fork { run_specs(run_descriptor) } # We don't use Process.waitpid here as it was causing bisects to - # block due to the file descriptor limit on OSX / Linux. + # block due to the file descriptor limit on OSX / Linux. We need + # to detach the process to avoid having zombie process and consume + # slot in the kernel process table. + Process.detach(pid) end private From bc4e6a0546c4b9dac4e98cbf92588aafc830160e Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Mon, 15 Jun 2020 11:07:12 +0100 Subject: [PATCH 02/11] Add spec to prevent zombie status --- spec/integration/bisect_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index 1d3a864391..e03220bb62 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -33,7 +33,7 @@ def bisect(cli_args, expected_status=nil) end end - context "when the bisect commasaturingnd is long" do + context "when the bisect command saturates the pipe" do # On OSX and Linux a file descriptor limit meant that the bisect process got stuck at a certain limit. # This test demonstrates that we can run large bisects above this limit (found to be at time of commit). # See: https://github.com/rspec/rspec-core/pull/2669 @@ -41,6 +41,11 @@ def bisect(cli_args, expected_status=nil) output = bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) expect(output).to include("No failures found.") end + + it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do + bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) + expect(%x[ps -ho pid,state]).to_not include("Z") + end end end end From c67f46266410ca8a873f772f6cb915b58014c173 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Mon, 15 Jun 2020 17:32:26 +0200 Subject: [PATCH 03/11] Changelog for #2739 --- Changelog.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Changelog.md b/Changelog.md index 5e3f84d20a..e877562007 100644 --- a/Changelog.md +++ b/Changelog.md @@ -14,6 +14,8 @@ Bug Fixes: * Ensure custom error codes are returned from bisect runs. (Jon Rowe, #2732) * Ensure `RSpec::Core::Configuration` predicate config methods return booleans. (Marc-André Lafortune, #2736) +* Prevent `rspec --bisect` from generating zombie processes while executing + (Benoit Tigeot, Jon Rowe, #2739) ### 3.9.2 / 2020-05-02 [Full Changelog](http://github.com/rspec/rspec-core/compare/v3.9.1...v3.9.2) From f9da1b7e9a2f03d735e116e9b954339bf45d3c2a Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Thu, 18 Jun 2020 11:23:58 +0100 Subject: [PATCH 04/11] Update Changelog.md --- Changelog.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Changelog.md b/Changelog.md index e877562007..020b853dc2 100644 --- a/Changelog.md +++ b/Changelog.md @@ -15,7 +15,7 @@ Bug Fixes: * Ensure `RSpec::Core::Configuration` predicate config methods return booleans. (Marc-André Lafortune, #2736) * Prevent `rspec --bisect` from generating zombie processes while executing - (Benoit Tigeot, Jon Rowe, #2739) + bisect runs. (Benoit Tigeot, Jon Rowe, #2739) ### 3.9.2 / 2020-05-02 [Full Changelog](http://github.com/rspec/rspec-core/compare/v3.9.1...v3.9.2) From 6e622fa4b9115de02be68841d8da35968bd7339a Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Thu, 18 Jun 2020 11:57:27 +0100 Subject: [PATCH 05/11] Further improve spec to focus on the recently exited bisect process --- spec/integration/bisect_spec.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index e03220bb62..a6cc539554 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -43,9 +43,15 @@ def bisect(cli_args, expected_status=nil) end it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do + original_pids = pids() bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) - expect(%x[ps -ho pid,state]).to_not include("Z") + sleep 0.1 while (extra_pids = pids() - original_pids).join.match?(/[RE]/i) + expect(extra_pids.join).to_not include "Z" end end + + def pids + %x[ps -ho pid,state].split("\n").map { |line| line.split(/\s+/).compact.join(' ') } + end end end From 9dfbc4525539997b198645fdbd7d08d15c3b0262 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 19 Jun 2020 14:37:10 +0200 Subject: [PATCH 06/11] Update lib/rspec/core/bisect/fork_runner.rb Co-authored-by: Jon Rowe --- lib/rspec/core/bisect/fork_runner.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/rspec/core/bisect/fork_runner.rb b/lib/rspec/core/bisect/fork_runner.rb index c00ab1d98b..4641a14429 100644 --- a/lib/rspec/core/bisect/fork_runner.rb +++ b/lib/rspec/core/bisect/fork_runner.rb @@ -94,8 +94,8 @@ def dispatch_specs(run_descriptor) pid = fork { run_specs(run_descriptor) } # We don't use Process.waitpid here as it was causing bisects to # block due to the file descriptor limit on OSX / Linux. We need - # to detach the process to avoid having zombie process and consume - # slot in the kernel process table. + # to detach the process to avoid having zombie processes + # consuming slots in the kernel process table during bisect runs. Process.detach(pid) end From f1ca0bbc914030e98bff7ac0ffdda9d2e9d22170 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Mon, 22 Jun 2020 23:02:40 +0200 Subject: [PATCH 07/11] Switch to pattern match sign for older rubies --- spec/integration/bisect_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index a6cc539554..1e2375fc4a 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -45,7 +45,7 @@ def bisect(cli_args, expected_status=nil) it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do original_pids = pids() bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) - sleep 0.1 while (extra_pids = pids() - original_pids).join.match?(/[RE]/i) + sleep 0.1 while (extra_pids = pids() - original_pids).join =~ /[RE]/i expect(extra_pids.join).to_not include "Z" end end From 194cfe275cbf9b703161382a614061f185fe3612 Mon Sep 17 00:00:00 2001 From: Jon Rowe Date: Mon, 29 Jun 2020 20:28:04 +0100 Subject: [PATCH 08/11] Wrap =~ operator in brackets --- spec/integration/bisect_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index 1e2375fc4a..9c9d82ae20 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -45,7 +45,7 @@ def bisect(cli_args, expected_status=nil) it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do original_pids = pids() bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) - sleep 0.1 while (extra_pids = pids() - original_pids).join =~ /[RE]/i + sleep 0.1 while ((extra_pids = pids() - original_pids).join =~ /[RE]/i) expect(extra_pids.join).to_not include "Z" end end From 338c1223320da17e0ebdb050031057c63c32b69e Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Mon, 29 Jun 2020 23:48:15 +0200 Subject: [PATCH 09/11] Search Ruby processes. Avoid infinite while loop on Ubuntu --- spec/integration/bisect_spec.rb | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index 9c9d82ae20..d2d223d4a5 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -45,13 +45,17 @@ def bisect(cli_args, expected_status=nil) it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do original_pids = pids() bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) - sleep 0.1 while ((extra_pids = pids() - original_pids).join =~ /[RE]/i) + while ((extra_pids = pids() - original_pids).join =~ /[RE]/i) + sleep 0.1 + end expect(extra_pids.join).to_not include "Z" end end def pids - %x[ps -ho pid,state].split("\n").map { |line| line.split(/\s+/).compact.join(' ') } + %x[ps -ho pid,state,cmd | grep "[r]uby"].split("\n").map do |line| + line.split(/\s+/).compact.join(' ') + end end end end From 8ff81d0e88fcd066baa0d919c0e21b9881a1a134 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Mon, 29 Jun 2020 23:49:41 +0200 Subject: [PATCH 10/11] Add basic circuit breaker if we detect extra ruby process --- spec/integration/bisect_spec.rb | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index d2d223d4a5..b3fd21a202 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -45,7 +45,10 @@ def bisect(cli_args, expected_status=nil) it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do original_pids = pids() bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) + cursor = 0 while ((extra_pids = pids() - original_pids).join =~ /[RE]/i) + raise "Extra process detected" if cursor > 10 + cursor += 1 sleep 0.1 end expect(extra_pids.join).to_not include "Z" From 07754b61af29b111da1b01d07bdf7c43f550d2f1 Mon Sep 17 00:00:00 2001 From: Benoit Tigeot Date: Fri, 28 Aug 2020 15:45:23 +0200 Subject: [PATCH 11/11] Detect zombie processes --- spec/integration/bisect_spec.rb | 50 +++++++++++++++++++++++++-------- 1 file changed, 38 insertions(+), 12 deletions(-) diff --git a/spec/integration/bisect_spec.rb b/spec/integration/bisect_spec.rb index b3fd21a202..820ca4d8c9 100644 --- a/spec/integration/bisect_spec.rb +++ b/spec/integration/bisect_spec.rb @@ -43,21 +43,47 @@ def bisect(cli_args, expected_status=nil) end it 'does not leave zombie processes', :unless => RSpec::Support::OS.windows? do - original_pids = pids() - bisect(%W[spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_], 1) - cursor = 0 - while ((extra_pids = pids() - original_pids).join =~ /[RE]/i) - raise "Extra process detected" if cursor > 10 - cursor += 1 - sleep 0.1 - end - expect(extra_pids.join).to_not include "Z" + bisect(['--format', 'json', 'spec/rspec/core/resources/blocking_pipe_bisect_spec.rb_'], 1) + + zombie_process = RSpecChildProcess.new(Process.pid).zombie_process + expect(zombie_process).to eq([]), <<-MSG + Expected no zombie processes got #{zombie_process.count}: + #{zombie_process} + MSG end end - def pids - %x[ps -ho pid,state,cmd | grep "[r]uby"].split("\n").map do |line| - line.split(/\s+/).compact.join(' ') + class RSpecChildProcess + Ps = Struct.new(:pid, :ppid, :state, :command) + + def initialize(pid) + @list = child_process_list(pid) + end + + def zombie_process + @list.select { |child_process| child_process.state =~ /Z/ } + end + + private + + def child_process_list(pid) + childs_process_list = [] + ps_pipe = `ps -o pid=,ppid=,state=,args= | grep #{pid}` + + ps_pipe.split(/\n/).map do |line| + ps_part = line.lstrip.split(/\s+/) + + next unless ps_part[1].to_i == pid + + child_process = Ps.new + child_process.pid = ps_part[0] + child_process.ppid = ps_part[1] + child_process.state = ps_part[2] + child_process.command = ps_part[3..-1].join(' ') + + childs_process_list << child_process + end + childs_process_list end end end