-
Notifications
You must be signed in to change notification settings - Fork 31.1k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
src: fix kill signal on Windows #55514
src: fix kill signal on Windows #55514
Conversation
070e103
to
3eaa41f
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #55514 +/- ##
==========================================
- Coverage 88.40% 88.40% -0.01%
==========================================
Files 653 654 +1
Lines 187437 187747 +310
Branches 36073 36126 +53
==========================================
+ Hits 165711 165978 +267
- Misses 14956 15006 +50
+ Partials 6770 6763 -7
|
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
|
||
// Test different types of kill signals on Windows. | ||
if (common.isWindows) { | ||
const process1 = spawn('cmd'); |
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.
Can this be simplified down to a for loop of test cases?
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.
Thank you for the review. Fixed it.
Is there anything else I can do to help this PR move forward? |
Commit Queue failed- Loading data for nodejs/node/pull/55514 ✔ Done loading data for nodejs/node/pull/55514 ----------------------------------- PR info ------------------------------------ Title src: fix kill signal on Windows (#55514) ⚠ Could not retrieve the email or name of the PR author's from user's GitHub profile! Branch huseyinacacak-janea:huseyin-10409-process-kill-signal -> nodejs:main Labels child_process, c++, needs-ci, commit-queue-squash Commits 4 - src: fix kill signal on Windows - Update doc/api/child_process.md - Update test/parallel/test-child-process-kill.js - fixup! src: fix kill signal on Windows Committers 2 - Hüseyin Açacak <huseyin@janeasystems.com> - GitHub <noreply@github.com> PR-URL: https://github.com/nodejs/node/pull/55514 Fixes: https://github.com/nodejs/node/issues/42923 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/55514 Fixes: https://github.com/nodejs/node/issues/42923 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> -------------------------------------------------------------------------------- ⚠ Commits were pushed since the last approving review: ⚠ - Update doc/api/child_process.md ⚠ - Update test/parallel/test-child-process-kill.js ⚠ - fixup! src: fix kill signal on Windows ℹ This PR was created on Thu, 24 Oct 2024 10:31:09 GMT ✔ Approvals: 1 ✔ - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/55514#pullrequestreview-2393535306 ✔ Last GitHub CI successful ℹ Last Full PR CI on 2024-11-12T10:37:04Z: https://ci.nodejs.org/job/node-test-pull-request/63526/ - Querying data for job/node-test-pull-request/63526/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncuhttps://github.com/nodejs/node/actions/runs/11893278060 |
Fixes: nodejs#42923 PR-URL: nodejs#55514 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
Fixes: nodejs#42923 PR-URL: nodejs#55514 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Stefan Stojanovic <stefan.stojanovic@janeasystems.com>
As stated in the documentation of Node.js, the
signal
parameter of child processkill
function is ignored.However, when we execute
spawn(process.execPath, ['-v']).kill('SIGHUP');
, it throws an exception with the codeENOSYS
. This is not consistent with the documentation.I've looked at the implementation in libuv and have seen that only four signal types are supported to kill a process on Windows.
This PR fixes this issue by changing the signal to
SIGKILL
if it differs fromSIGQUIT
,SIGTERM
,SIGKILL
, andSIGINT
. In addition, there is a slight change in the documentation to clarify thesignal
parameter.Fixes: #42923