Skip to content

Commit

Permalink
Ctrl-C handling by rescuing interrupt
Browse files Browse the repository at this point in the history
By not installing global trap handlers, we prevent any conflicts with any
other installed handlers. This means that, for example, when running cli
or runner specs, one can interrupt rubocop and get the expected Ctrl-C
handling (RSpec's one). It also means that rubocop's Ctrl-C handling is
never leaked outside of rubocop, like it happens when running puma's
test suite.
  • Loading branch information
deivid-rodriguez authored and bbatsov committed Mar 17, 2019
1 parent 06ef4c2 commit 6aff4c8
Show file tree
Hide file tree
Showing 4 changed files with 59 additions and 45 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,7 @@
* [#6594](https://github.com/rubocop-hq/rubocop/pull/6594): Fix a false positive for `Rails/OutputSafety` when the receiver is a non-interpolated string literal. ([@amatsuda][])
* [#6574](https://github.com/rubocop-hq/rubocop/pull/6574): Fix `Style/AccessModifierIndentation` not handling arbitrary blocks. ([@deivid-rodriguez][])
* [#6370](https://github.com/rubocop-hq/rubocop/issues/6370): Fix the enforcing style from `extend self` into `module_function` when there are private methods. ([@Ruffeng][])
* [#6461](https://github.com/rubocop-hq/rubocop/pull/6461): Restrict Ctrl-C handling to RuboCop's loop and simplify it to a single phase. ([@deivid-rodriguez][])

### Changes

Expand Down
19 changes: 6 additions & 13 deletions lib/rubocop/runner.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,6 @@ def initialize(options, config_store)
@aborting = false
end

def trap_interrupt
Signal.trap('INT') do
exit!(1) if aborting?
self.aborting = true
warn ''
warn 'Exiting... Interrupt again to exit immediately.'
end
end

def run(paths)
target_files = find_target_files(paths)
if @options[:list_target_files]
Expand All @@ -47,6 +38,12 @@ def run(paths)
warm_cache(target_files) if @options[:parallel]
inspect_files(target_files)
end
rescue Interrupt
self.aborting = true
warn ''
warn 'Exiting...'

false
end

def aborting?
Expand Down Expand Up @@ -81,11 +78,7 @@ def inspect_files(files)
end

def each_inspected_file(files)
trap_interrupt

files.reduce(true) do |all_passed, file|
break false if aborting?

offenses = process_file(file)
yield file

Expand Down
9 changes: 6 additions & 3 deletions spec/rubocop/formatter/base_formatter_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -100,9 +100,12 @@ def file_finished(_file, _offenses)
end
end

allow(runner).to receive(:aborting?) do
formatter.processed_file_count == 2
end
allow(runner).to receive(:process_file)
.and_wrap_original do |m, *args|
raise Interrupt if formatter.processed_file_count == 2

m.call(*args)
end

run

Expand Down
75 changes: 46 additions & 29 deletions spec/rubocop/runner_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,49 +12,66 @@ class Runner
let(:formatter_output_path) { 'formatter_output.txt' }
let(:formatter_output) { File.read(formatter_output_path) }

describe '#trap_interrupt' do
context '#run when interrupted' do
include_context 'cli spec behavior'

let(:runner) { described_class.new({}, RuboCop::ConfigStore.new) }
let(:interrupt_handlers) { [] }

before do
allow(Signal).to receive(:trap).with('INT') do |&block|
interrupt_handlers << block
end
end

def interrupt
interrupt_handlers.each(&:call)
create_empty_file('example.rb')
end

it 'adds a handler for SIGINT' do
expect(interrupt_handlers.empty?).to be(true)
runner.trap_interrupt
expect(interrupt_handlers.size).to eq(1)
def interrupt(pid)
Process.kill 'INT', pid
end

context 'with SIGINT once' do
it 'aborts processing' do
runner.trap_interrupt
def wait_for_input(io)
line = nil

expect { interrupt }.to change(runner, :aborting?).from(false).to(true)
until line
line = io.gets
sleep 0.1
end

it 'does not exit immediately' do
runner.trap_interrupt
expect_any_instance_of(Object).not_to receive(:exit)
expect_any_instance_of(Object).not_to receive(:exit!)
interrupt
end
line
end

context 'with SIGINT twice' do
it 'exits immediately' do
runner.trap_interrupt
expect_any_instance_of(Object).to receive(:exit!).with(1)
interrupt
interrupt
around do |example|
old_handler = Signal.trap('INT', 'DEFAULT')
example.run
Signal.trap('INT', old_handler)
end

context 'with SIGINT' do
it 'returns false' do
skip unless Process.respond_to?(:fork)

# Make sure the runner works slowly and thus is interruptible
allow(runner).to receive(:process_file) do
sleep 99
end

rd, wr = IO.pipe

pid = Process.fork do
rd.close
wr.puts 'READY'
wr.puts runner.run(['example.rb'])
wr.close
end

wr.close

# Make sure the runner has started by waiting for a specific message
line = wait_for_input(rd)
expect(line.chomp).to eq('READY')

# Interrupt the runner
interrupt(pid)

# Make sure the runner returns false
line = wait_for_input(rd)
expect(line.chomp).to eq('false')
end
end
end
Expand Down

0 comments on commit 6aff4c8

Please sign in to comment.