Merge pull request #10046 from reitermarkus/sigint

Improve handling of SIGINT.
This commit is contained in:
Markus Reiter 2020-12-18 17:44:30 +01:00 committed by GitHub
commit 89e5d80d18
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 127 additions and 60 deletions

View File

@ -32,14 +32,10 @@ module Homebrew
ruby_sys_args << "-e #{args.e}" if args.e
ruby_sys_args += args.named
begin
safe_system RUBY_PATH,
ENV["HOMEBREW_RUBY_WARNINGS"],
"-I", $LOAD_PATH.join(File::PATH_SEPARATOR),
"-rglobal", "-rdev-cmd/irb",
*ruby_sys_args
rescue ErrorDuringExecution => e
exit e.status.exitstatus
end
exec RUBY_PATH,
ENV["HOMEBREW_RUBY_WARNINGS"],
"-I", $LOAD_PATH.join(File::PATH_SEPARATOR),
"-rglobal", "-rdev-cmd/irb",
*ruby_sys_args
end
end

View File

@ -578,14 +578,24 @@ class ErrorDuringExecution < RuntimeError
@status = status
@output = output
exitstatus = if status.respond_to?(:exitstatus)
status.exitstatus
else
exitstatus = case status
when Integer
status
else
status.exitstatus
end
redacted_cmd = redact_secrets(cmd.shelljoin.gsub('\=', "="), secrets)
s = +"Failure while executing; `#{redacted_cmd}` exited with #{exitstatus}."
reason = if exitstatus
"exited with #{exitstatus}"
elsif (uncaught_signal = status.termsig)
"was terminated by uncaught signal #{Signal.signame(uncaught_signal)}"
else
raise ArgumentError, "Status does neither have `exitstatus` nor `termsig`."
end
s = +"Failure while executing; `#{redacted_cmd}` #{reason}."
if Array(output).present?
format_output_line = lambda do |type_line|

View File

@ -20,6 +20,8 @@ class SystemCommand
# Helper functions for calling {SystemCommand.run}.
module Mixin
extend T::Sig
def system_command(*args)
T.unsafe(SystemCommand).run(*args)
end
@ -32,8 +34,6 @@ class SystemCommand
include Context
extend Predicable
attr_reader :pid
def self.run(executable, **options)
T.unsafe(self).new(executable, **options).run!
end
@ -44,7 +44,7 @@ class SystemCommand
sig { returns(SystemCommand::Result) }
def run!
puts redact_secrets(command.shelljoin.gsub('\=', "="), @secrets) if verbose? || debug?
$stderr.puts redact_secrets(command.shelljoin.gsub('\=', "="), @secrets) if verbose? || debug?
@output = []
@ -152,28 +152,42 @@ class SystemCommand
end
end
def each_output_line(&b)
sig { params(block: T.proc.params(type: Symbol, line: String).void).void }
def each_output_line(&block)
executable, *args = command
options = {
# Create a new process group so that we can send `SIGINT` from
# parent to child rather than the child receiving `SIGINT` directly.
pgroup: true,
}
options[:chdir] = chdir if chdir
raw_stdin, raw_stdout, raw_stderr, raw_wait_thr =
T.unsafe(Open3).popen3(env, [executable, executable], *args, **{ chdir: chdir }.compact)
@pid = raw_wait_thr.pid
pid = T.let(nil, T.nilable(Integer))
raw_stdin, raw_stdout, raw_stderr, raw_wait_thr = ignore_interrupts do
T.unsafe(Open3).popen3(env, [executable, executable], *args, **options)
.tap { |*, wait_thr| pid = wait_thr.pid }
end
write_input_to(raw_stdin)
raw_stdin.close_write
each_line_from [raw_stdout, raw_stderr], &b
each_line_from [raw_stdout, raw_stderr], &block
@status = raw_wait_thr.value
rescue Interrupt
Process.kill("INT", pid) if pid
raise Interrupt
rescue SystemCallError => e
@status = $CHILD_STATUS
@output << [:stderr, e.message]
end
sig { params(raw_stdin: IO).void }
def write_input_to(raw_stdin)
input.each(&raw_stdin.method(:write))
end
def each_line_from(sources)
sig { params(sources: T::Array[IO], _block: T.proc.params(type: Symbol, line: String).void).void }
def each_line_from(sources, &_block)
loop do
readable_sources, = IO.select(sources)

View File

@ -165,34 +165,37 @@ RSpec.configure do |config|
config.around do |example|
def find_files
return [] unless File.exist?(TEST_TMPDIR)
Find.find(TEST_TMPDIR)
.reject { |f| File.basename(f) == ".DS_Store" }
.reject { |f| TEST_DIRECTORIES.include?(Pathname(f)) }
.map { |f| f.sub(TEST_TMPDIR, "") }
end
Homebrew.raise_deprecation_exceptions = true
Formulary.clear_cache
Tap.clear_cache
DependencyCollector.clear_cache
Formula.clear_cache
Keg.clear_cache
Tab.clear_cache
FormulaInstaller.clear_attempted
FormulaInstaller.clear_installed
TEST_DIRECTORIES.each(&:mkpath)
@__homebrew_failed = Homebrew.failed?
@__files_before_test = find_files
@__env = ENV.to_hash # dup doesn't work on ENV
@__stdout = $stdout.clone
@__stderr = $stderr.clone
begin
Homebrew.raise_deprecation_exceptions = true
Formulary.clear_cache
Tap.clear_cache
DependencyCollector.clear_cache
Formula.clear_cache
Keg.clear_cache
Tab.clear_cache
FormulaInstaller.clear_attempted
FormulaInstaller.clear_installed
TEST_DIRECTORIES.each(&:mkpath)
@__homebrew_failed = Homebrew.failed?
@__files_before_test = find_files
@__env = ENV.to_hash # dup doesn't work on ENV
@__stdout = $stdout.clone
@__stderr = $stderr.clone
if (example.metadata.keys & [:focus, :byebug]).empty? && !ENV.key?("VERBOSE_TESTS")
$stdout.reopen(File::NULL)
$stderr.reopen(File::NULL)
@ -224,7 +227,7 @@ RSpec.configure do |config|
Tab.clear_cache
FileUtils.rm_rf [
TEST_DIRECTORIES.map(&:children),
*TEST_DIRECTORIES,
*Keg::MUST_EXIST_SUBDIRECTORIES,
HOMEBREW_LINKED_KEGS,
HOMEBREW_PINNED_KEGS,

View File

@ -24,7 +24,11 @@ describe SystemCommand do
it "includes the given variables explicitly" do
expect(Open3)
.to receive(:popen3)
.with(an_instance_of(Hash), ["/usr/bin/env", "/usr/bin/env"], "A=1", "B=2", "C=3", "env", *env_args, {})
.with(
an_instance_of(Hash), ["/usr/bin/env", "/usr/bin/env"], "A=1", "B=2", "C=3",
"env", *env_args,
pgroup: true
)
.and_call_original
command.run!
@ -49,8 +53,10 @@ describe SystemCommand do
it "includes the given variables explicitly" do
expect(Open3)
.to receive(:popen3)
.with(an_instance_of(Hash), ["/usr/bin/sudo", "/usr/bin/sudo"], "-E", "--",
"/usr/bin/env", "A=1", "B=2", "C=3", "env", *env_args, {})
.with(
an_instance_of(Hash), ["/usr/bin/sudo", "/usr/bin/sudo"], "-E", "--",
"/usr/bin/env", "A=1", "B=2", "C=3", "env", *env_args, pgroup: true
)
.and_wrap_original do |original_popen3, *_, &block|
original_popen3.call("true", &block)
end
@ -257,24 +263,45 @@ describe SystemCommand do
context "when given arguments with secrets" do
it "does not leak the secrets" do
redacted_msg = /#{Regexp.escape("username:******")}/
expect do
expect {
described_class.run! "curl",
args: %w[--user username:hunter2],
verbose: true,
secrets: %w[hunter2]
end.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stdout
}.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stderr
end
it "does not leak the secrets set by environment" do
redacted_msg = /#{Regexp.escape("username:******")}/
expect do
expect {
ENV["PASSWORD"] = "hunter2"
described_class.run! "curl",
args: %w[--user username:hunter2],
verbose: true
ensure
ENV.delete "PASSWORD"
end.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stdout
}.to raise_error.with_message(redacted_msg).and output(redacted_msg).to_stderr
end
end
context "when a `SIGINT` handler is set in the parent process" do
it "is not interrupted" do
start_time = Time.now
pid = fork do
trap("INT") do
# Ignore SIGINT.
end
described_class.run! "sleep", args: [5]
exit!
end
sleep 1
Process.kill("INT", pid)
Process.waitpid(pid)
expect(Time.now - start_time).to be >= 5
end
end
end

View File

@ -388,13 +388,30 @@ module Kernel
Pathname.new(cmd).archs
end
def ignore_interrupts(opt = nil)
std_trap = trap("INT") do
puts "One sec, just cleaning up" unless opt == :quietly
def ignore_interrupts(_opt = nil)
# rubocop:disable Style/GlobalVars
$ignore_interrupts_nesting_level = 0 unless defined?($ignore_interrupts_nesting_level)
$ignore_interrupts_nesting_level += 1
$ignore_interrupts_interrupted = false unless defined?($ignore_interrupts_interrupted)
old_sigint_handler = trap(:INT) do
$ignore_interrupts_interrupted = true
$stderr.print "\n"
$stderr.puts "One sec, cleaning up..."
end
yield
ensure
trap("INT", std_trap)
begin
yield
ensure
trap(:INT, old_sigint_handler)
$ignore_interrupts_nesting_level -= 1
if $ignore_interrupts_nesting_level == 0 && $ignore_interrupts_interrupted
$ignore_interrupts_interrupted = false
raise Interrupt
end
end
# rubocop:enable Style/GlobalVars
end
sig { returns(String) }