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.

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

Properly handle interrupt signals #17

Merged
merged 2 commits into from
Aug 16, 2022
Merged

Properly handle interrupt signals #17

merged 2 commits into from
Aug 16, 2022

Conversation

felddy
Copy link
Member

@felddy felddy commented Aug 15, 2022

🗣 Description

This PR fixes an issue with signal handling within interactive shells of child processes. Specifically,
pressing control-c in a shell causes awssh to exit instead of allowing the shell to interpret the signal.

💭 Motivation and context

Currently, control-c will exit awssh which ends a user's remote session.
This is broken and infuriating. By setting an ignore action on the interrupt signal, in the parent,
before thesubprocess call, the child can then handle the signal as expected.
The signal's action in the parent is restored after the child exits for good measure.

🧪 Testing

Tested on my local machine by connecting to various AWS instances and furiously mashing control-c.
Tested with standard suite of continuous integration actions.

monday-typing-monkey

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

✅ Post-merge checklist

  • Add a tag or create a release.

@felddy felddy added bug This issue or pull request addresses broken functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use labels Aug 15, 2022
@felddy felddy self-assigned this Aug 15, 2022
@felddy felddy marked this pull request as ready for review August 15, 2022 21:51
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice and tidy. 👍
Thanks for looking into this and fixing it!

Tickle GitHub actions #2.
@felddy felddy merged commit c48c3c4 into develop Aug 16, 2022
@felddy felddy deleted the improvement/signals branch August 16, 2022 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This issue or pull request addresses broken functionality improvement This issue or pull request will add or improve functionality, maintainability, or ease of use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants