Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve Ctrl-C handling #6461

Merged
merged 2 commits into from Mar 17, 2019
Merged

Improve Ctrl-C handling #6461

merged 2 commits into from Mar 17, 2019

Conversation

deivid-rodriguez
Copy link
Contributor

@deivid-rodriguez deivid-rodriguez commented Nov 8, 2018 •
edited

This PR builds on top of #6452 and improves the user experience when clicking Ctrl-C by not installing any global trap handlers, and thus:

  • Not leaking the handler to the rest of the process after rubocop is done.
  • Keeping the previous global handlers in use when rubocop starts. For example, when clicking Ctrl-C during the execution of rubocop CLI specs, one gets the expected handling of Ctrl-C. You can see the comparison of the before and after on this screen recording.

I felt it was better to propose these changes as two separate PRs, so technically this PR depends on #6452. But all changes in both PRs are included here, so if it's handier or considered better, this PR could be reviewed and merged direcly as a single set of changes, and thus superseding #6452.

Before submitting the PR make sure the following are checked:

  • Wrote good commit messages.
  • Commit message starts with [Fix #issue-number] (if the related issue exists).
  • Feature branch is up-to-date with master (if not - rebase it).
  • Squashed related commits together.
  • Added tests.
  • Added an entry to the Changelog if the new code introduces user-observable changes. See changelog entry format.
  • The PR relates to only one subject with a clear title
    and description in grammatically correct, complete sentences.
  • Run bundle exec rake default. It executes all tests and RuboCop for itself, and generates the documentation.

@deivid-rodriguez deivid-rodriguez mentioned this pull request Nov 9, 2018
Closed
8 tasks
@deivid-rodriguez
Copy link
Contributor Author

I rebased this PR after #6537 being merged. This PR still removes the 2-phase Ctrl-C handling but does not suffer from the problem explained in #6452 (comment).

@deivid-rodriguez
Copy link
Contributor Author

I rebased this one again, any thoughts? :)

@mikegee
Copy link
Contributor

mikegee commented Dec 12, 2018

I prefer rescue Interrupt over Signal.trap('INT'). 👍

@deivid-rodriguez
Copy link
Contributor Author

Rebased again and added a changelog entry.

@mikegee
Copy link
Contributor

mikegee commented Dec 24, 2018

rescue Interrupt over Signal.trap('INT')

new cop mebbe?

@deivid-rodriguez
Copy link
Contributor Author

Maybe... In my experience, rescue Interrupt usually results in better behaved programs, but I'm not sure if this is something that should be recommended in general.

@deivid-rodriguez
Copy link
Contributor Author

@mikegee I'm actually using Signal.trap to test this PR, so I definitely don't think we should recommend against Signal.trap in general.

@deivid-rodriguez deivid-rodriguez changed the title Improve Ctrl-c handling Improve Ctrl-C handling Dec 27, 2018
@deivid-rodriguez
Reuse cli spec behavior context
908db03
@deivid-rodriguez
Ctrl-C handling by rescuing interrupt
2dd246d 
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.
@bbatsov
Copy link
Collaborator

bbatsov commented Mar 17, 2019

@mikegee I'm actually using Signal.trap to test this PR, so I definitely don't think we should recommend against Signal.trap in general.

Can use capture other signals without Signal.trap?

@bbatsov bbatsov merged commit 6aff4c8 into rubocop:master Mar 17, 2019
@deivid-rodriguez
Copy link
Contributor Author

deivid-rodriguez commented Mar 23, 2019 •
edited

Yeah, @mikegee was referring only to Signal.trap("INT")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@deivid-rodriguez @mikegee @bbatsov