-
Notifications
You must be signed in to change notification settings - Fork 360
fix(run): correct SIGINT forwarding #4552
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
fix(run): correct SIGINT forwarding #4552
Conversation
e9451ed
to
f4a3c1b
Compare
f4a3c1b
to
6cb1e2a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
look good to me at the first glance - will play with it tomorrow.
I wonder, do we need a separate test for this?
Yes, it would be good to make a test out of the nice reproducer in #4426. If the difference, between non-TTY (CI) and TTY makes this too annoying, I'd be okay with leaving the test for now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can confirm that this fixes the reproducer
I just tried this fix on my example from #4532 (comment) and it worked as expected, I only got one SIGINT 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Valentin!
Should fix #4426, but won't solve the problem completely.
We have the following cases:
This PR fixes the first two cases; the third case already had a solution, leaving the fourth. To solve it, we should get PGID of the subprocess (using its PID), but I'm not aware of a way to do this with
deno_task_shell
. After spending some time looking through its code base, I couldn't figure out any proper way to implement this, so postponed complete solution.