child_process: support numeric signal argument to kill. by gerrard00 · Pull Request #9651 · nodejs/node

gerrard00 · 2016-11-17T01:24:28Z

Checklist

make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

child_process

Description of change

Modifies the kill function to support numeric signals. I used a type check instead of Number.isInteger because that was the pattern used elsewhere in the code base.

Addresses: #9519

addaleax · 2016-11-17T01:29:57Z

Awesome! Do you think you could get a test for this together? There’s a bit on that in https://github.com/nodejs/node/blob/master/doc/guides/writing_tests.md, but I’d assume there are existing tests for the string signal names in test/parallel that you can add to.

And a few tiny commit message things: Could you drop the . at the end of the subject line, try to squeeze the subject line so that it fits into 50 characters (I know it’s hard with child_process… 😄) and add a Fixes: https://github.com/nodejs/node/issues/9519 line?

gerrard00 · 2016-11-17T01:42:01Z

Will do!

addaleax · 2016-11-17T01:47:10Z

Thanks, apart from the missing tests this looks good!

gerrard00 · 2016-11-17T01:57:17Z

There was an existing test for child-process-kill, so I made a copy of that test and modified it to send the numeric value of SIGTERM.

I also modified the commit message as requested. I totally forgot to check that length before pushing.

cjihrig · 2016-11-17T15:28:12Z

Couldn't this replace the if (sig === 0) check a few lines up?

Oh, that makes more sense. Updated.

cjihrig · 2016-11-17T15:28:50Z

Please use const instead of var throughout the test.

cjihrig · 2016-11-17T15:30:24Z

Could you use common.spawnCat()?

cjihrig · 2016-11-17T15:31:21Z

Please add common.mustCall() around the callback, then gotStdoutEOF can be completely removed.

cjihrig · 2016-11-17T15:31:37Z

Please use common.fail() here.

cjihrig · 2016-11-17T15:32:05Z

Please add common.mustCall() around the callback, then gotStderrEOF can be completely removed.

cjihrig · 2016-11-17T15:32:51Z

Please use common.mustCall(), and move the assertions for exitCode and termSignal here.

cjihrig · 2016-11-17T15:33:04Z

assert.strictEqual() please.

cjihrig · 2016-11-17T15:33:20Z

assert.strictEqual() again here, please.

cjihrig · 2016-11-17T15:33:41Z

Once the common.mustCall() changes are made, this can be removed completely.

gerrard00 · 2016-11-17T16:04:30Z

@cjihrig Those changes are easy enough, but that will make this test very different than the existing child process kill test. Is that ok?

cjihrig · 2016-11-17T16:07:35Z

The existing test is outdated, even if it's still technically correct. You can also check out the relatively new https://github.com/nodejs/node/blob/master/doc/guides/writing_tests.md.

gerrard00 · 2016-11-17T17:48:09Z

@cjihrig I think I've updated the test correctly. Can you take another look when you have a moment?

Thanks for your patience!

sam-github · 2016-11-17T18:51:12Z

commit messages should not have a . at the end of them

gerrard00 · 2016-11-17T18:59:10Z

@sam-github The period in the commit message was removed based on feedback from @addaleax .

sam-github

The commit is missing documentation.

sam-github · 2016-11-17T18:52:12Z

@cjihrig should we enforce integral, so 1.2 is not acceptable?

@sam-github makes sense to me.

sam-github · 2016-11-17T18:54:32Z

did you test this? I'm pretty sure that 0 is now going to become SIGTERM.

Ouch! That's totally wrong. I didn't test 0 after making the suggested change. Will fix.

sam-github · 2016-11-17T18:58:52Z

I think this could be constants.os.signals.SIGTERM, so the comment would be unnecessary.

sam-github

this makes child.kill() mysteriously different from process.kill(), doesn't it? They should accept the same arguments.

targos · 2016-11-17T18:59:27Z

This condition has to be after typeof sig === 'number' otherwise signal 0 will be incorrectly converted to SIGTERM

sam-github · 2016-11-17T19:01:55Z

+1 on supporting numeric signals

sam-github · 2016-11-17T19:04:17Z

I didn't look around, but if there are existing tests for kill, it would be good to add this to them, it can be hard to find tests when there is one big test file that mostly tests a feature the a couple smaller ones sprinkled about that test the incremental additions that were made over time.

^---- I didn't look around, I'm just at the github review tool right now, if its not the case that there is a good test to extend, and this new test file is the best way, pls ignore.

gerrard00 · 2016-11-17T19:30:52Z

@sam-github My lunch break is over, but if I can get the child_process change to an acceptable state modifying process to support the numeric the same way should be easy.

addaleax

LGTM

gerrard00 · 2016-11-17T23:56:05Z

@sam-github I took a cut at updating the docs and fixed a typo in that area.

cjihrig · 2016-11-18T04:39:44Z

String|number -> String|Number

Fixed! That was actually a c+p from process.md, so I'll add another commit to fix that file. Or should I open a different PR for that?

cjihrig · 2016-11-18T04:44:06Z

if there are existing tests for kill, it would be good to add this to them

The test from this PR could be integrated with test/parallel/test-child-process-kill.js fairly easily.

gerrard00 · 2016-11-18T06:09:54Z

I'll combine the new test with the existing child process test tomorrow.

sam-github · 2016-11-18T14:50:58Z

Btw, I still object to adding numeric signal support to child.kill(), and not to process.kill(). Doing so creates an instantaneous bug report: "child.kill accepts numberic signals but process.kill does not". IMO, a missing feature with no bugs is better than a half-feature + a bug report. I request that either you open two PRs simultaneously, put two commits in this PR, or just do both in one commit, so the node API incrementally gets better in a single commit, instead of slightly better and slightly worse.

Also, I'm surprised child_process does its own arg checking at all, instead of calling https://github.com/nodejs/node/blob/master/lib/internal/process.js#L152-L178, so that the arg checks, Error strings, etc. would be provably identical.

I don't have time to look further, but it looks to me like _handle.kill could be replaced with require('internal/process').kill, unifying code and behaviour.

jasnell · 2016-11-18T17:12:42Z

This should likely check that the value is not an invalid number. For instance:
typeof NaN === 'number' typeof Infinity === 'number' typeof 1.1 === 'number' typeof -1 === 'number'

typeof sig === 'number' && sig >>> 0 === sig should probably do the trick.

Awesome, you just taught me something. I've recently written similar code and used a bunch of || conditions.
Modified as requested.

gerrard00 · 2016-11-21T17:52:33Z

@cjihrig I combined the tests as request, can you take a look?

@sam-github I'll look into using require('internal/process').kill and modify the code if it works. I'll also open a second PR to modify process and link to it from this PR once I've had a chance to test that.

cjihrig

LGTM pending CI

Fixes: nodejs#9519

Fishrock123 · 2016-12-16T15:03:23Z

@sam-github @targos could you take a look and update your reviews accordingly?

Fishrock123 · 2016-12-16T15:06:42Z

@@ -1,4 +1,4 @@
-# Child Process
+#n Child Process


@gerrard00 You have an extra n here. :)
We can fix that on landing though.

Fishrock123 · 2016-12-16T15:07:52Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5440/

targos · 2016-12-16T15:24:28Z

-The `child.kill()` methods sends a signal to the child process. If no argument
-is given, the process will be sent the `'SIGTERM'` signal. See signal(7) for
+The `child.kill()` method sends a signal to the child process. If no argument
+is given, the process will be sent the `'SIGTERM'` signal. See `signal(7)` for


I think we don't put backticks around man page references

is this getting fixed?

targos · 2016-12-16T15:25:53Z

+ }));
+
+ assert.equal(cat.killed, false);
+ cat.kill();


Shouldn't it pass sig here?

targos · 2016-12-16T15:27:56Z

+}
+
+runTest(constants.os.signals.SIGTERM);
+runTest('SIGTERM');


a third test can be added without an argument (to check the default value for signal)

sam-github · 2016-12-19T18:16:30Z

My comment from #9651 (comment) still stands:

I still object to adding numeric signal support to child.kill(), and not to process.kill(). Doing so creates an instantaneous bug report: "child.kill accepts numeric signals but process.kill does not". IMO, a missing feature with no bugs is better than a half-feature + a bug report.

There is no reason to make these two .kill() APIs inconsistent. Fix them both in one PR.

Landing these kind of changes has historically been a source of node API weirdness, wherein two parts of the API subtlely diverge. There is no reason to do that here, lets just make it all better, consistently, at one time.

Fishrock123 · 2016-12-21T23:37:58Z

@sam-github Ah, yes. I agree on that.

Trott · 2017-03-19T22:04:20Z

@gerrard00 Are you still interested in getting this across the finish line?

Adding stalled for now, but if anyone (@gerrard00 or someone else) is actively working on it, please remove that label.

gerrard00 · 2017-03-21T15:35:12Z

I'll close this and work on a new PR based on the feedback provided.

thefourtheye · 2017-03-21T16:12:08Z

Actually #10423 covers implicitly this as well.

nodejs-github-bot added the child_process Issues and PRs related to the child_process subsystem. label Nov 17, 2016

gerrard00 mentioned this pull request Nov 17, 2016
n.kill(19) throws error #9519
Closed

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Nov 17, 2016

gerrard00 force-pushed the support_numeric_kill branch from ae7c0d4 to 29198d4Compare November 17, 2016 01:43

gerrard00 force-pushed the support_numeric_kill branch from 29198d4 to 410c589Compare November 17, 2016 01:55

cjihrig reviewed Nov 17, 2016
View reviewed changes

cjihrig requested changes Nov 17, 2016
View reviewed changes

gerrard00 force-pushed the support_numeric_kill branch 5 times, most recently from 044fdec to 7c08ce9Compare November 17, 2016 17:46

sam-github suggested changes Nov 17, 2016
View reviewed changes

targos requested changes Nov 17, 2016
View reviewed changes

gerrard00 force-pushed the support_numeric_kill branch 2 times, most recently from 9ba3620 to 798fe20Compare November 17, 2016 19:24

addaleax approved these changes Nov 17, 2016
View reviewed changes

gerrard00 force-pushed the support_numeric_kill branch from 798fe20 to 5e8cdf9Compare November 17, 2016 23:55

cjihrig reviewed Nov 18, 2016
View reviewed changes

gerrard00 force-pushed the support_numeric_kill branch from 5e8cdf9 to 7c45e72Compare November 18, 2016 06:07

jasnell reviewed Nov 18, 2016
View reviewed changes

gerrard00 force-pushed the support_numeric_kill branch 2 times, most recently from 76e52b8 to 04a8e74Compare November 21, 2016 17:47

cjihrig approved these changes Nov 21, 2016
View reviewed changes

child_process: support numeric signal arg to kill
fd7a620
Fixes: nodejs#9519

gerrard00 force-pushed the support_numeric_kill branch from 04a8e74 to fd7a620Compare November 22, 2016 02:02

Fishrock123 self-assigned this Dec 16, 2016

Fishrock123 reviewed Dec 16, 2016
View reviewed changes

targos reviewed Dec 16, 2016
View reviewed changes

Trott added the stalled Issues and PRs that are stalled. label Mar 19, 2017

gerrard00 closed this Mar 21, 2017

Uh oh!

Uh oh!

Conversation

gerrard00 commented Nov 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

addaleax commented Nov 17, 2016

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

addaleax commented Nov 17, 2016

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerrard00Nov 17, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

cjihrig commented Nov 17, 2016

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

sam-github commented Nov 17, 2016

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sam-github commented Nov 17, 2016

Uh oh!

sam-github commented Nov 17, 2016

Uh oh!

gerrard00 commented Nov 17, 2016

Uh oh!

addaleax left a comment

gerrard00 commented Nov 17, 2016•
edited
Loading

gerrard00Nov 17, 2016•
edited
Loading