From 2cb7a09eb00f4e5a1712e9ce12c0b1b800eba623 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 22f8b5fbfe3f55f035476b6c822924b129ed6b3c 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 20be067792e7af2f566c8e73194799e048b21022 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 57d97477a7336b5d5adcae132a26bc153bedbacd 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 fa50b75358e1472c79506480567bab040b03502b 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 7c0f2c45b5c6b99a5c2253b8cfacfa1f4907921d 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 cde31ee1a4ba08eec322d7be935a51ada1d42b53 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 008c7171b9f31f40ac19fb049fb1fcdeced637a2 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 ce8b6c67fbcce30f5e6c34cad0da31760e183a77 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 35a47f309a621e4d35cc51f51da6835dd585b590 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 4494cec92c5f018c26c1d4235706209c8937348b 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