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

feat(exec) ignore ctrl-c when exec #171

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

sadayuki-matsuno
Copy link
Contributor

#170

Description of changes:

ignore ctrl-c in pet exec

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@RamiAwar
Copy link
Collaborator

Thinking about this again to cover edge cases:

1- If user runs a process with Pet, they expect any following keyboard events to go to that process. Pet should not be intervening anymore.

2- The second a sub-process ends, Pet will also stop. We don't have to worry about having Pet hang here I think. Only concern is that the sub-processs hangs which is the user's responsibility.

3- Do we need to have this be under a flag? -> No because there is no expectation from users that Ctrl+C will end Pet, the parent process that launched this subprocess. Users don't even know that Pet is still running and shouldn't care about this.

SO I think we can just merge this as is now!

Copy link
Collaborator

@RamiAwar RamiAwar left a comment

Choose a reason for hiding this comment

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

Tested this locally, and it doesn't seem enough. It doesn't communicate the Ctrl+C command to the subprocess.

Tried this with sleep:

Command: sleep 10

Then launch it with pet exec, and it doesn't work as expected. Ctrl+C does nothing. We need to find away around this still sadly, but we'll figure it out!

If you test this with current Pet however, cancelling the sleep works fine. I guess it has to do with the fact that the subprocess is not accepting inputs in 'sleep'. But if launching an SSH subprocess, it probably will work.

Hmmm..

Back to my points above, I think this affects (3):

  • Do we need to have this under a flag? -> YES because some subprocesses might not be accepting inputs, and we might want to end them. If they were running normally (outside of Pet, sleep 10) you can always end with ctrl+c. This should still be the case when using Pet by default.

For cases like SSH, would make sense to put this under a flag where these events get passed through to the subprocess.

Thoughts? @sadayuki-matsuno

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants