lib: allow custom names for spawned processes by ppannuto · Pull Request #7696 · nodejs/node

ppannuto · 2016-07-13T02:16:38Z

Checklist

make -j4 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)

lib/child_process.js

Description of change

Add a name parameter to the options accepted by
child_process.spawn that sets the spawned process name (argv[0])
to a custom value. The default behavior is unchanged if a name
parameter is not supplied.

It is useful to be able to set the name of a process you are spawning.
One motivating example is an application runner service that spawn
several other node applications. Instead of showing up as 20 different
node processes, the application runner can give them meaningful names.

While child processes can write process.title to change their name,
this has two limitations:

it requires modifying all child processes
it is limited to the length of the initial argv[0] in practice

mscdex · 2016-07-13T02:27:13Z

Perhaps this should be a bit more specific, like typeof options.name === 'string'.

thefourtheye · 2016-07-13T03:43:46Z

Better to be use common.mustCall to wrap the callback function

thefourtheye · 2016-07-13T06:56:56Z

LGTM. CI Run: https://ci.nodejs.org/job/node-test-pull-request/3277/

Edit: Changes look LGTM. We can still hear from others how useful they think this feature would be.

evanlucas · 2016-07-13T11:42:25Z

I'd be curious to see if this works on SmartOS. IIRC, SmartOS does not support changing process.title

evanlucas · 2016-07-13T11:46:20Z

Yea, looks like tests are failing on Windows and SmartOS

ppannuto · 2016-07-13T14:28:56Z

Interesting. It appears that there's no mechanism to change the process name on Windows ( http://stackoverflow.com/questions/23783985/set-child-process-name-in-windows ).

I've added a note to the documentation that this isn't supported on all platforms and added check that skips this test for Windows and SunOS.

trevnorris · 2016-07-13T15:40:02Z

Indentation alright here?

Oops. fixed.

evanlucas · 2016-07-13T15:52:22Z

I do think we should be cautious about adding features that are not supported on all platforms.

saghul · 2016-07-13T15:57:18Z

IMHO this is not the right way to do it. Node already supports process.title = "foo"; which uses the libuv provided facilities (uv_set_process_title in this case) to do that. This works on Windows too AFAIK.

Now, if you want to do that after spawning a process you'd need to run that code in the child. A quick idea would be to have a NODE_PROCESS_TITLE env variable, which Node reads on startup and uses to set the effective process title.

Please don't follow this blindly, maybe there is a better option. I'd go for actually not doing anything. If you want your workers to have a different process title set some WORKER_NAME env variable and call process.title = ... in your app code.

cjihrig · 2016-07-13T15:58:15Z

I agree with both @evanlucas and @saghul

ppannuto · 2016-07-13T15:58:23Z

Perhaps this is starting to evolve into a nomenclature issue then. My personal motivation was controlling process name, but in general, the ability to control the argv[0] passed to a child process can be useful for some applications.

This patch would eliminate hacky-er things such as https://github.com/andrewffff/child_process_with_argv0 or needless intermediaries like https://github.com/andrep/argv0

Would this be more amenable as an option named argv0 in the array?

saghul · 2016-07-13T15:58:50Z

it is limited to the length of the initial argv[0] in practice

I don't have all platforms to test right now, but are you certain about that?

ppannuto · 2016-07-13T16:00:07Z

@saghul on my OS X laptop for example:

$ node
> process.title
'node'
> process.title = "longerTitle"
'longerTitle'
> process.title
'long'
$ psgrep long
USER PID %CPU %MEM VSZ RSS TT STAT STARTED TIME COMMAND
ppannuto 48334 0.0 0.1 3139124 23828 s007 T 11:59AM 0:00.12 long

ppannuto · 2016-07-13T16:00:49Z

I should qualify that as "often limited" or "sometimes limited", it's not universally limited (i.e. Linux always allows 16 characters IIRC)

ppannuto · 2016-07-13T16:04:37Z

As for having the workers change their process title, that is explicitly against my current goals which is to not have to modify the workers at all (i.e. they shouldn't know they're being run by a runner service and not being invoked directly, nor more importantly should a service need to be modified to be supported by this runner).

addaleax · 2016-07-13T16:05:12Z

@ppannuto I think it’s still the length of the entire argv array that counts, because that’s the space that’s available for it.

@saghul Keep in mind that this applies not only to Node.js child processes. I have actually been wondering why Node.js doesn’t support setting argv[0] manually; I know that some Unix CLIs actually inspect it and make decisions based on it.

Also, that specifying argv[0] lets one override the Node.js process title is more of a coincidence, and I am not sure name is the right identifier here.

ppannuto · 2016-07-13T16:06:56Z

@addaleax you are correct that it's full argv array that's accessible to process.title

I'll update the PR to call the option argv0 and note that it has the side effect of changing the process title on some platforms in the documentation. Sound good?

addaleax · 2016-07-13T16:08:52Z

I'll update the PR to call the option argv0 and note that it has the side effect of changing the process title on some platforms in the documentation. Sound good?

That does sound better to me, yes.

ppannuto · 2016-07-29T17:09:48Z

@jasnell think I got everything. Lmk if anything else needs to change.

addaleax · 2016-07-29T19:32:31Z

New CI: https://ci.nodejs.org/job/node-test-commit/4326/

jasnell · 2016-08-01T18:38:09Z

s/_Note:/_Note*:
For this one and the next. :-)

Every other "Note" in the file currently italicizes the whole note ( https://github.com/nodejs/node/blame/master/doc/api/child_process.md#L26, https://github.com/nodejs/node/blame/master/doc/api/child_process.md#L39, https://github.com/nodejs/node/blame/master/doc/api/child_process.md#L188 etc). I'm happy to change this one instance if you want, but should it really be inconsistent with the rest of the file?

Ok, no worries. I'll go through and fix those in a separate PR :-)

jasnell · 2016-08-01T18:40:54Z

Awesome... thank you @ppannuto. I left a couple more comments but I think this is just about ready to go!

For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - nodejs#7434 - nodejs#7449 - nodejs#7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0`

In some cases it useful to control the value of `argv[0]`, c.f. - https://github.com/andrewffff/child_process_with_argv0 - https://github.com/andrep/argv0 This patch adds explicit support for setting the value of `argv[0]` when spawning a process.

ppannuto · 2016-08-01T18:48:40Z

Updated. I didn't currently change the Note italics, but can if you'd like.

jasnell · 2016-08-04T20:42:56Z

Thank you! LGTM!

jasnell · 2016-08-04T20:43:33Z

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

addaleax · 2016-08-08T14:04:44Z

New new CI (that last one failed on Windows): https://ci.nodejs.org/job/node-test-commit/4447/

For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - #7434 - #7449 - #7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0` PR-URL: #7696 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

In some cases it useful to control the value of `argv[0]`, c.f. - https://github.com/andrewffff/child_process_with_argv0 - https://github.com/andrep/argv0 This patch adds explicit support for setting the value of `argv[0]` when spawning a process. PR-URL: #7696 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax · 2016-08-08T14:42:53Z

Landed in a804db1, 99f45b2. Thanks for the PR! I believe this is your first commit to core, if so, welcome on board and we hope you find other ways to contribute!

For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - #7434 - #7449 - #7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0` PR-URL: #7696 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

In some cases it useful to control the value of `argv[0]`, c.f. - https://github.com/andrewffff/child_process_with_argv0 - https://github.com/andrep/argv0 This patch adds explicit support for setting the value of `argv[0]` when spawning a process. PR-URL: #7696 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) nodejs#7983 and nodejs#7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) nodejs#7811 and nodejs#7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) nodejs#7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) nodejs#7942 * repl: The REPL now supports editor mode. (Prince J Wesley) nodejs#7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) nodejs#8013 Refs: nodejs#8020 PR-URL: nodejs#8070

gibfahn · 2017-02-16T03:03:17Z

@MylesBorins is there a reason this was never backported to v4?

MylesBorins · 2017-02-16T17:51:35Z

@gibfahn it was never brought up. Semver minors need to be proposed

gibfahn · 2017-02-17T01:46:51Z

@MylesBorins okay, didn't realise that. Is this how I propose that it be backported?

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

mscdex reviewed Jul 13, 2016
View reviewed changes

ppannuto force-pushed the spawn_process_name branch from 32435ca to 46f188aCompare July 13, 2016 03:16

thefourtheye reviewed Jul 13, 2016
View reviewed changes

ppannuto force-pushed the spawn_process_name branch from 46f188a to 1489f88Compare July 13, 2016 04:42

targos added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 13, 2016

evanlucas reviewed Jul 13, 2016
View reviewed changes

ppannuto force-pushed the spawn_process_name branch from 1489f88 to 5fd63e8Compare July 13, 2016 14:27

trevnorris reviewed Jul 13, 2016
View reviewed changes

ppannuto force-pushed the spawn_process_name branch from 5fd63e8 to 2d72e34Compare July 13, 2016 15:51

ppannuto force-pushed the spawn_process_name branch from 2d72e34 to df6140dCompare July 13, 2016 15:53

jasnell reviewed Aug 1, 2016
View reviewed changes

ppannuto added 2 commits August 1, 2016 14:45

process: save original argv[0]
de074fe
For historical and other reasons, node overwrites `argv[0]` on startup. See - 2c6f79c, - nodejs#7434 - nodejs#7449 - nodejs#7696 For cases where it may be useful, save the original value of `argv[0]` in `process.argv0`

ppannuto force-pushed the spawn_process_name branch from d361830 to c9880e3Compare August 1, 2016 18:48

addaleax closed this Aug 8, 2016

cjihrig mentioned this pull request Aug 8, 2016
v6.4.0 Release Proposal #8020
Closed

ppannuto deleted the spawn_process_name branch August 8, 2016 19:25

cjihrig mentioned this pull request Aug 11, 2016
v6.4.0 Proposal #8070
Merged

sam-github mentioned this pull request Feb 16, 2017
Define Policy regarding Semver-Minor LTS Releases nodejs/Release#171
Closed

gibfahn added the lts-watch-v4.x label Feb 17, 2017

Uh oh!

Uh oh!

Conversation

ppannuto commented Jul 13, 2016

Checklist

Affected core subsystem(s)

Description of change

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!

thefourtheye commented Jul 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Jul 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppannuto commented Jul 13, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Jul 13, 2016

Uh oh!

saghul commented Jul 13, 2016

Uh oh!

cjihrig commented Jul 13, 2016

Uh oh!

ppannuto commented Jul 13, 2016

Uh oh!

saghul commented Jul 13, 2016

Uh oh!

ppannuto commented Jul 13, 2016

Uh oh!

ppannuto commented Jul 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppannuto commented Jul 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Jul 13, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppannuto commented Jul 13, 2016

Uh oh!

addaleax commented Jul 13, 2016

Uh oh!

ppannuto commented Jul 29, 2016

Uh oh!

addaleax commented Jul 29, 2016

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!

jasnell commented Aug 1, 2016

Uh oh!

ppannuto commented Aug 1, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

jasnell commented Aug 4, 2016

Uh oh!

addaleax commented Aug 8, 2016

Uh oh!

addaleax commented Aug 8, 2016

Uh oh!

gibfahn commented Feb 16, 2017

Uh oh!

MylesBorins commented Feb 16, 2017

thefourtheye commented Jul 13, 2016•
edited
Loading

evanlucas commented Jul 13, 2016•
edited
Loading

ppannuto commented Jul 13, 2016•
edited
Loading

ppannuto commented Jul 13, 2016•
edited
Loading

addaleax commented Jul 13, 2016•
edited
Loading