Skip to content

subprocess.kill should ignore signal argument on Windows #42923

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

Closed
fasttime opened this issue Apr 29, 2022 · 4 comments · Fixed by #55514
Closed

subprocess.kill should ignore signal argument on Windows #42923

fasttime opened this issue Apr 29, 2022 · 4 comments · Fixed by #55514
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.

Comments

@fasttime
Copy link
Contributor

fasttime commented Apr 29, 2022

Version

v18.0.0

Platform

Microsoft Windows NT 10.0.19044.0 x64

Subsystem

child_process

What steps will reproduce the bug?

The documentation about subprocess.kill([signal]) says:

On Windows, where POSIX signals do not exist, the signal argument will be
ignored, and the process will be killed forcefully and abruptly (similar to
'SIGKILL').

This does not seem to match the current behavior, where the signal argument is not ignored on Windows, and it can lead in fact to different results. A minimal repro:

// test.mjs - Run on Windows

import { spawn } from 'child_process';

spawn(process.execPath, ['-v']).kill('SIGKILL'); // ok
spawn(process.execPath, ['-v']).kill('SIGHUP'); // throws ENOSYS

How often does it reproduce? Is there a required condition?

Always on Windows

What is the expected behavior?

If the documentation is right, invoking kill('SIGHUP') on a subprocess on Windows should work exactly like kill('SIGKILL').

What do you see instead?

The signal argument is not currently ignored on Windows. I can't decide if this behavior is intended and the documentation is wrong or if the documentation is right and the behavior is incorrect.

Additional information

If someone could clarify me about the expectations of running subprocess.kill on Windows, I would be glad to submit a PR to fix the code or the documentation.

@VoltrexKeyva VoltrexKeyva added child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform. labels Apr 30, 2022
@fabiospampinato
Copy link

This seems a fairly significant issue, in the sense that anybody that is triggering a signal with process.kill without knowing this issue is probably doing it in a problematic way for Windows.

huseyinacacak-janea added a commit to JaneaSystems/node that referenced this issue Oct 24, 2024
nodejs-github-bot pushed a commit that referenced this issue Nov 20, 2024
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
tpoisseau pushed a commit to tpoisseau/node that referenced this issue Nov 21, 2024
Fixes: nodejs#42923
PR-URL: nodejs#55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
Ceres6 pushed a commit to Ceres6/node that referenced this issue Nov 26, 2024
Fixes: nodejs#42923
PR-URL: nodejs#55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
aduh95 pushed a commit that referenced this issue Nov 26, 2024
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
aduh95 pushed a commit that referenced this issue Dec 10, 2024
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
ruyadorno pushed a commit that referenced this issue Jan 5, 2025
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jan 22, 2025
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jan 22, 2025
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jan 22, 2025
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
marco-ippolito pushed a commit that referenced this issue Jan 24, 2025
Fixes: #42923
PR-URL: #55514
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Stefan Stojanovic <[email protected]>
@ehmicky
Copy link

ehmicky commented Mar 28, 2025

Documenting the change of behavior for anyone encountering it.

Windows does not really have the Unix concept of signals. So Node emulates it.
Node on Windows only supports the following signals:

  • SIGINT, SIGQUIT, SIGTERM, SIGKILL: all have the behavior, i.e. to terminate the process immediately.
  • SIGHUP, SIGILL, SIGABRT, SIGFPE, SIGSEGV, SIGWINCH, SIGBREAK: are present in os.constants.signals, can be listened to, but cannot be sent with subprocess.kill().

Before Node 23.4.0, calling subprocess.kill('SIGHUP') (and SIGILL, etc.) used to emit an error event on the subprocess, with error.code 'ENOSYS'. However, it does not terminate the subprocess.

Since Node 23.4.0, the signal is converted to SIGKILL instead. Which means the subprocess will not emit an error event, but end with an exit/close event. Also, the subprocess is terminated immediately. The event handler will receive (null, 'SIGKILL') as its (exitCode, signalCode) arguments.

@ehmicky
Copy link

ehmicky commented Mar 28, 2025

@fabiospampinato @fasttime I am not sure this PR was made realizing the following points:

  • This impacts not only SIGHUP but some other signals which are never meant for termination, such as SIGWINCH.
  • The subprocess is now terminated abruptly. It did not used to. Note: emitting an error event on the subprocess does not terminate it (unless it has not been spawned yet).

The second point in particular is quite a breaking change.

I can see how a user sending SIGHUP would probably want to terminate the subprocess abruptly on Windows instead (since Windows does not support such signals). However, a user sending SIGWINCH most likely does not want to terminate the subprocess on Windows.

It seems to me:

  1. This change should have been in a major release, since it is technically breaking.
  2. Although calling subprocess.kill('SIGWINCH') rarely makes sense, that specific signal should still be ignored instead of terminating the subprocess abruptly.

@ehmicky
Copy link

ehmicky commented Mar 28, 2025

Additionally, subprocess.kill(0) is now broken. I opened a separate issue for it at #57669 since it is a different problem that the one I was mentioned above, and might require a different solution.

StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Mar 31, 2025
This special case was missed in the previous changes to this file.

Refs: nodejs#55514
Refs: nodejs#42923
Fixes: nodejs#57669
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Mar 31, 2025
This special case was missed in the previous changes to this file.

Refs: nodejs#55514
Refs: nodejs#42923
Fixes: nodejs#57669
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Mar 31, 2025
This special case was missed in the previous changes to this file.

Refs: nodejs#55514
Refs: nodejs#42923
Fixes: nodejs#57669
StefanStojanovic added a commit to JaneaSystems/node that referenced this issue Apr 3, 2025
This special case was missed in the previous changes to this file.

Refs: nodejs#55514
Refs: nodejs#42923
Fixes: nodejs#57669
nodejs-github-bot pushed a commit that referenced this issue Apr 4, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
JonasBa pushed a commit to JonasBa/node that referenced this issue Apr 11, 2025
This special case was missed in the previous changes to this file.

Refs: nodejs#55514
Refs: nodejs#42923
Fixes: nodejs#57669
PR-URL: nodejs#57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this issue May 1, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this issue May 2, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
aduh95 pushed a commit that referenced this issue May 6, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
RafaelGSS pushed a commit that referenced this issue May 14, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
aduh95 pushed a commit that referenced this issue May 17, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
aduh95 pushed a commit that referenced this issue May 17, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
aduh95 pushed a commit that referenced this issue May 19, 2025
This special case was missed in the previous changes to this file.

Refs: #55514
Refs: #42923
Fixes: #57669
PR-URL: #57695
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Jake Yuesong Li <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants