Skip to content

runtests-watch: Don't try to listen for SIGKILL #54114

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

Merged

Conversation

Peeja
Copy link
Contributor

@Peeja Peeja commented May 3, 2023

It's not possible (because a SIGKILL ends the process immediately, before the process can do anything to respond to it), and in current versions of Node attempting to will throw Error: uv_signal_start EINVAL. Thus, removing this "listener" removes no actual functionality; it only stops the task from immediately erroring on newer versions of Node.

See also: https://stackoverflow.com/questions/16311347/node-script-throws-uv-signal-start-einval

Fixes #54113

(I've ignored a bunch of PR process that didn't seem to apply to a tooling fix like this, but please let me know if there's anything I should do differently.)

It's not possible (because a SIGKILL ends the process immediately,
before the process can do anything to respond to it), and in current
versions of Node attempting to will throw `Error: uv_signal_start
EINVAL`. Thus, removing this "listener" removes no actual functionality;
it only stops the task from immediately erroring on newer versions of
Node.

See also: https://stackoverflow.com/questions/16311347/node-script-throws-uv-signal-start-einval
@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label May 3, 2023
@jakebailey
Copy link
Member

@rbuckton Do you know if this handler is still needed? I suspect it isn't, but I'm trying to think as to whether or not this is needed for Windows or something.

@rbuckton
Copy link
Contributor

rbuckton commented May 3, 2023

@rbuckton Do you know if this handler is still needed? I suspect it isn't, but I'm trying to think as to whether or not this is needed for Windows or something.

The specific handler being removed? Probably not.

@jakebailey
Copy link
Member

Thanks.

I think I'll just merge this; I think that if it's crashing, that's not good, and this mode is somewhat experimental, but SIGKILL definitely can't be listened to.

@jakebailey jakebailey merged commit 7c378db into microsoft:main May 3, 2023
@Peeja
Copy link
Contributor Author

Peeja commented May 3, 2023

Thanks, all!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

runtests-watch runs nothing in modern Node
4 participants