Skip to content
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

Rename ChildProcess exec and fork to something which is not entirely confusing to system developers #224

Closed
guss77 opened this issue Dec 31, 2014 · 12 comments
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.

Comments

@guss77
Copy link

guss77 commented Dec 31, 2014

The ChildProcess module's exec and fork methods sound a bit similar to the system calls of that name, but behave entirely differently. I've been bit by this a few times already.

To reduce confusion and make it easier to understand what is going on, these should be renamed to something like similarly functional methods in other environements (for example exec I believe is very similar to system, OTOH I'm not sure what I would do with fork).

Also, it would be great if IO.js will add implementation for fork and exec system calls so that developers can take advantage of these powerfull primitives to create their own multi-process mechanisms.

@qfox
Copy link

qfox commented Dec 31, 2014

Totally agreed. But personally I'm already accustomed to this naming ;-(

@DavidSouther
Copy link

System programers should change their process execution model to be something sensible ;)

Only sort of J/K - the unix process model is not exactly "intuitive" IMHO. But I totally agree that the current names in ChildProcess are inappropriate.

@sam-github
Copy link
Contributor

I found it confusing, too, but any suggestion of changing should include a proposal for new names! I suspect they are what they are because no one could think of better at the time.

@DavidSouther
Copy link

@sam-github Fair enough.

  • ChildProcess.spawn - as current; equivalent to posix fork / exec.
  • ChildProcess.execute - as ChildProcess.exec; delegates to a POSIX shell.
  • ChildProcess.start - As ChildProcess.execFile; delegates to a POSIX shell with a -c flag.
  • ChildProcess.run - As ChildProcess.fork; shortcut to starting a new v8 node/iojs process.

@sam-github
Copy link
Contributor

Hm, well

  • execute is just longer than exec, but might guide people away from thinking it's equivalent to exec(2). BTW, it doesn't delegate to a POSIX shell on Windows, its cmd.exe there, which is most of the value
  • start: doesn't use a shell on any system, its purpose in life is not use a shell, it directly spawns the executable file. This name, start, fails to indicate that it has basically the same interface as exec/execute, in that it runs to completion and returns the output, and differs only in not having an intermediate shell.
  • run: isn't any more descriptive than fork(), but at least doesn't leave people thinking it is fork(2) equivalent

I hated the names at first, too, but at this point, I think the biggest benefit would be an introduction added to the child_process docs, summarizing the use-cases for the various functions, with particular emphasis on portability (why spawn and execFile won't work for scripts, such as node scripts, on Windows).

@qfox
Copy link

qfox commented Dec 31, 2014

The best thread is here ;-D 🎄

Agree. Looks like we need some documentation to understand all cases we have.

@garthk
Copy link

garthk commented Jan 1, 2015

With @sam-github: unless we're suddenly embracing breaking API changes, fixing this with documentation is a good approach.

Is there some general principles document from the TC giving the relative priority of "not breaking" and "forward"?

@dashed
Copy link

dashed commented Jan 3, 2015

+1 for documentation clarification.

@trevnorris
Copy link
Contributor

A documentation fix sounds like the appropriate solution. We won't be breaking API any time soon, so we're stuck with the names.

@trevnorris trevnorris added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Jan 16, 2015
@brendanashworth brendanashworth added the good first issue Issues that are suitable for first-time contributors. label May 16, 2015
@Trott
Copy link
Member

Trott commented May 17, 2015

Is a simple one-line note like done here sufficient? https://github.com/nodejs/io.js/pull/1718/files

Or should the differences be explicitly stated? (And if so, is that a requirement or merely something that would be done ideally but not strictly necessary?)

@guss77
Copy link
Author

guss77 commented May 17, 2015

I would think that something with more content would be preferable, noting
the major differences without giving uninterested parties an unrequested
crash course in system programming. Something like:

child_process.exec() is unrelated to the exec() system call in
Unix-like operating systems. This call is similar to the system() Unix
call in that it does not replace the existing process and uses a shell to
execute the command.

and

child_process.fork() is unrelated to the fork() system call in
Unix-like operating systems. This call is similar to the "fork-exec"
technique used in Unix system programs.

On Sun, May 17, 2015 at 5:27 PM, Rich Trott [email protected]
wrote:

Is a simple one-line note like done here sufficient?
https://github.com/nodejs/io.js/pull/1718/files

Or should the differences be explicitly stated? (And if so, is that a
requirement or merely something that would be done ideally but not strictly
necessary?)


Reply to this email directly or view it on GitHub
#224 (comment).

Oded

silverwind pushed a commit that referenced this issue May 20, 2015
Adds notes about the difference to their POSIX counterparts.

PR-URL: #1718
Fixes: #224
Reviewed-By: Roman Reiss <[email protected]>
@silverwind
Copy link
Contributor

Fixed by 86dd244

andrewdeandrade pushed a commit to andrewdeandrade/node that referenced this issue Jun 3, 2015
Adds notes about the difference to their POSIX counterparts.

PR-URL: nodejs/node#1718
Fixes: nodejs/node#224
Reviewed-By: Roman Reiss <[email protected]>
Trott pushed a commit to npm/node that referenced this issue Aug 20, 2019
Full release notes:

A few meaty bugfixes, and introducing `peerDependenciesMeta`.

FEATURES

* [`a12341088`](npm/cli@a123410)
  [nodejs#224](npm/cli#224) Implements
  peerDependenciesMeta ([@arcanis](https://github.com/arcanis))
* [`2f3b79bba`](npm/cli@2f3b79b)
  [nodejs#234](npm/cli#234) add new forbidden 403
  error code ([@claudiahdz](https://github.com/claudiahdz))

BUGFIXES

* [`24acc9fc8`](npm/cli@24acc9f)
  and
  [`45772af0d`](npm/cli@45772af)
  [nodejs#217](npm/cli#217)
  [npm.community#8863](https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863)
  [npm.community#9327](https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327,)
  do not descend into directory deps' child modules, fix shrinkwrap
  files that inappropriately list child nodes of symlink packages
  ([@isaacs](https://github.com/isaacs) and
  [@salomvary](https://github.com/salomvary))
* [`50cfe113d`](npm/cli@50cfe11)
  [nodejs#229](npm/cli#229) fixed typo in semver doc
  ([@gall0ws](https://github.com/gall0ws))
* [`e8fb2a1bd`](npm/cli@e8fb2a1)
  [nodejs#231](npm/cli#231) Fix spelling mistakes in
  CHANGELOG-3.md ([@XhmikosR](https://github.com/XhmikosR))
* [`769d2e057`](npm/cli@769d2e0)
  [npm/uid-number#7](npm/uid-number#7) Better
  error on invalid `--user`/`--group` configs. This addresses the issue
  when people fail to install binary packages on Docker and other
  environments where there is no 'nobody' user.
  ([@isaacs](https://github.com/isaacs))
* [`8b43c9624`](npm/cli@8b43c96)
  [nodejs#28987](nodejs#28987)
  [npm.community#6032](https://npm.community/t/npm-ci-doesnt-respect-npmrc-variables/6032)
  [npm.community#6658](https://npm.community/t/npm-ci-doesnt-fill-anymore-the-process-env-npm-config-cache-variable-on-post-install-scripts/6658)
  [npm.community#6069](https://npm.community/t/npm-ci-does-not-compile-native-dependencies-according-to-npmrc-configuration/6069)
  [npm.community#9323](https://npm.community/t/npm-6-9-x-not-passing-environment-to-node-gyp-regression-from-6-4-x/9323/2)
  Fix the regression where random config values in a .npmrc file are not
  passed to lifecycle scripts, breaking build processes which rely on
  them.  ([@isaacs](https://github.com/isaacs))
* [`8b85eaa47`](npm/cli@8b85eaa)
  save files with inferred ownership rather than relying on `SUDO_UID`
  and `SUDO_GID`. ([@isaacs](https://github.com/isaacs))
* [`b7f6e5f02`](npm/cli@b7f6e5f)
  Infer ownership of shrinkwrap files
  ([@isaacs](https://github.com/isaacs))
* [`54b095d77`](npm/cli@54b095d)
  [nodejs#235](npm/cli#235) Add spec to dist-tag
  remove function ([@theberbie](https://github.com/theberbie))

DEPENDENCIES

* [`dc8f9e52f`](npm/cli@dc8f9e5)
  `[email protected]`: Infer the ownership of all unpacked files in
  `node_modules`, so that we never have user-owned files in root-owned
  folders, or root-owned files in user-owned folders.
  ([@isaacs](https://github.com/isaacs))
* [`bb33940c3`](npm/cli@bb33940)
  `[email protected]`:
  * [`9c93ac3`](npm/cmd-shim@9c93ac3)
    [#2](npm/cmd-shim#2)
    [npm#3380](npm/npm#3380) Handle
    environment variables properly
    ([@basbossink](https://github.com/basbossink))
  * [`2d277f8`](npm/cmd-shim@2d277f8)
    [nodejs#25](npm/cmd-shim#25)
    [nodejs#36](npm/cmd-shim#36)
    [nodejs#35](npm/cmd-shim#35) Fix 'no shebang' case
    by always providing `$basedir` in shell script
    ([@igorklopov](https://github.com/igorklopov))
  * [`adaf20b`](npm/cmd-shim@adaf20b)
    [nodejs#26](npm/cmd-shim#26) Fix `$*` causing an
    error when arguments contain parentheses
    ([@satazor](https://github.com/satazor))
  * [`49f0c13`](npm/cmd-shim@49f0c13)
    [nodejs#30](npm/cmd-shim#30) Fix paths for
    MSYS/MINGW bash ([@dscho](https://github.com/dscho))
  * [`51a8af3`](npm/cmd-shim@51a8af3)
    [nodejs#34](npm/cmd-shim#34) Add proper support
    for PowerShell ([@ExE-Boss](https://github.com/ExE-Boss))
  * [`4c37e04`](npm/cmd-shim@4c37e04)
    [#10](npm/cmd-shim#10) Work around quoted
    batch file names ([@isaacs](https://github.com/isaacs))
* [`a4e279544`](npm/cli@a4e2795)
  `[email protected]` ([@isaacs](https://github.com/isaacs)):
    * fail properly if `uid-number` raises an error
    * [`7086a1809`](npm/cli@7086a18)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`8845141f9`](npm/cli@8845141)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`51c028215`](npm/cli@51c0282)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`534a5548c`](npm/cli@534a554)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`3038f2fd5`](npm/cli@3038f2f)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`a609a1648`](npm/cli@a609a16)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`f0346f754`](npm/cli@f0346f7)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`ca9c615c8`](npm/cli@ca9c615)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`b417affbf`](npm/cli@b417aff)
  `[email protected]` ([@isaacs](https://github.com/isaacs))

TESTS

* [`b6df0913c`](npm/cli@b6df091)
  [nodejs#228](npm/cli#228) Proper handing of
  /usr/bin/node lifecycle-path test
  ([@olivr70](https://github.com/olivr70))
* [`aaf98e88c`](npm/cli@aaf98e8)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
isaacs added a commit to npm/node that referenced this issue Aug 21, 2019
Full changelog:

6.11.1 (2019-08-20):

Fix a regression for windows command shim syntax.

* [`37db29647`](npm/cli@37db296)
  `[email protected]` ([@isaacs](https://github.com/isaacs))

v6.11.0 (2019-08-20):

A few meaty bugfixes, and introducing `peerDependenciesMeta`.

FEATURES

* [`a12341088`](npm/cli@a123410)
  [nodejs#224](npm/cli#224) Implements
  peerDependenciesMeta ([@arcanis](https://github.com/arcanis))
* [`2f3b79bba`](npm/cli@2f3b79b)
  [nodejs#234](npm/cli#234) add new forbidden 403 error
  code ([@claudiahdz](https://github.com/claudiahdz))

BUGFIXES

* [`24acc9fc8`](npm/cli@24acc9f)
  and
  [`45772af0d`](npm/cli@45772af)
  [nodejs#217](npm/cli#217)
  [npm.community#8863](https://npm.community/t/installing-the-same-module-under-multiple-relative-paths-fails-on-linux/8863)
  [npm.community#9327](https://npm.community/t/reinstall-breaks-after-npm-update-to-6-10-2/9327,)
  do not descend into directory deps' child modules, fix shrinkwrap files
  that inappropriately list child nodes of symlink packages
  ([@isaacs](https://github.com/isaacs) and
  [@salomvary](https://github.com/salomvary))
* [`50cfe113d`](npm/cli@50cfe11)
  [nodejs#229](npm/cli#229) fixed typo in semver doc
  ([@gall0ws](https://github.com/gall0ws))
* [`e8fb2a1bd`](npm/cli@e8fb2a1)
  [nodejs#231](npm/cli#231) Fix spelling mistakes in
  CHANGELOG-3.md ([@XhmikosR](https://github.com/XhmikosR))
* [`769d2e057`](npm/cli@769d2e0)
  [npm/uid-number#7](npm/uid-number#7) Better
  error on invalid `--user`/`--group` configs. This addresses the issue
  when people fail to install binary packages on Docker and other
  environments where there is no 'nobody' user.
  ([@isaacs](https://github.com/isaacs))
* [`8b43c9624`](npm/cli@8b43c96)
  [nodejs#28987](nodejs#28987)
  [npm.community#6032](https://npm.community/t/npm-ci-doesnt-respect-npmrc-variables/6032)
  [npm.community#6658](https://npm.community/t/npm-ci-doesnt-fill-anymore-the-process-env-npm-config-cache-variable-on-post-install-scripts/6658)
  [npm.community#6069](https://npm.community/t/npm-ci-does-not-compile-native-dependencies-according-to-npmrc-configuration/6069)
  [npm.community#9323](https://npm.community/t/npm-6-9-x-not-passing-environment-to-node-gyp-regression-from-6-4-x/9323/2)
  Fix the regression where random config values in a .npmrc file are not
  passed to lifecycle scripts, breaking build processes which rely on them.
  ([@isaacs](https://github.com/isaacs))
* [`8b85eaa47`](npm/cli@8b85eaa)
  save files with inferred ownership rather than relying on `SUDO_UID` and
  `SUDO_GID`. ([@isaacs](https://github.com/isaacs))
* [`b7f6e5f02`](npm/cli@b7f6e5f)
  Infer ownership of shrinkwrap files
  ([@isaacs](https://github.com/isaacs))
* [`54b095d77`](npm/cli@54b095d)
  [nodejs#235](npm/cli#235) Add spec to dist-tag remove
  function ([@theberbie](https://github.com/theberbie))

DEPENDENCIES

* [`dc8f9e52f`](npm/cli@dc8f9e5)
  `[email protected]`: Infer the ownership of all unpacked files in
  `node_modules`, so that we never have user-owned files in root-owned
  folders, or root-owned files in user-owned folders.
  ([@isaacs](https://github.com/isaacs))
* [`bb33940c3`](npm/cli@bb33940)
  `[email protected]`:
  * [`9c93ac3`](npm/cmd-shim@9c93ac3)
    [#2](npm/cmd-shim#2)
    [npm#3380](npm/npm#3380) Handle environment
    variables properly ([@basbossink](https://github.com/basbossink))
  * [`2d277f8`](npm/cmd-shim@2d277f8)
    [nodejs#25](npm/cmd-shim#25)
    [nodejs#36](npm/cmd-shim#36)
    [nodejs#35](npm/cmd-shim#35) Fix 'no shebang' case by
    always providing `$basedir` in shell script
    ([@igorklopov](https://github.com/igorklopov))
  * [`adaf20b`](npm/cmd-shim@adaf20b)
    [nodejs#26](npm/cmd-shim#26) Fix `$*` causing an
    error when arguments contain parentheses
    ([@satazor](https://github.com/satazor))
  * [`49f0c13`](npm/cmd-shim@49f0c13)
    [nodejs#30](npm/cmd-shim#30) Fix paths for MSYS/MINGW
    bash ([@dscho](https://github.com/dscho))
  * [`51a8af3`](npm/cmd-shim@51a8af3)
    [nodejs#34](npm/cmd-shim#34) Add proper support for
    PowerShell ([@ExE-Boss](https://github.com/ExE-Boss))
  * [`4c37e04`](npm/cmd-shim@4c37e04)
    [#10](npm/cmd-shim#10) Work around quoted
    batch file names ([@isaacs](https://github.com/isaacs))
* [`a4e279544`](npm/cli@a4e2795)
  `[email protected]` ([@isaacs](https://github.com/isaacs)):
    * fail properly if `uid-number` raises an error
* [`7086a1809`](npm/cli@7086a18)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`8845141f9`](npm/cli@8845141)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`51c028215`](npm/cli@51c0282)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`534a5548c`](npm/cli@534a554)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`3038f2fd5`](npm/cli@3038f2f)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`a609a1648`](npm/cli@a609a16)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`f0346f754`](npm/cli@f0346f7)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`ca9c615c8`](npm/cli@ca9c615)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
* [`b417affbf`](npm/cli@b417aff)
  `[email protected]` ([@isaacs](https://github.com/isaacs))

TESTS

* [`b6df0913c`](npm/cli@b6df091)
  [nodejs#228](npm/cli#228) Proper handing of
  /usr/bin/node lifecycle-path test
  ([@olivr70](https://github.com/olivr70))
* [`aaf98e88c`](npm/cli@aaf98e8)
  `[email protected]` ([@isaacs](https://github.com/isaacs))
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. doc Issues and PRs related to the documentations. good first issue Issues that are suitable for first-time contributors.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants