Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

cluster: rename worker.destroy() to worker.kill() again #4133

Closed
bnoordhuis opened this issue Oct 13, 2012 · 3 comments
Closed

cluster: rename worker.destroy() to worker.kill() again #4133

bnoordhuis opened this issue Oct 13, 2012 · 3 comments
Labels
Milestone

Comments

@bnoordhuis
Copy link
Member

It was renamed from kill() to destroy() in 5f08c3c for no apparent reason AFAICT.

To keep the mental disconnect with the child_process module as small as possible I want to name it (or alias as) kill() again.

/cc @AndreasMadsen

@AndreasMadsen
Copy link
Member

There is a couple of reasons however nothing solid.

  • The worker.destory() don't take a signal child.kill() dose.
  • When dealing with workers it is quite important to indicate that this is a bad "shutdown" (to use a 3th work), .destroy() indicates that. It translate to the socket.destory().

In general the main decision I made in the cluster refactor was about "should this match child_process.fork or the net.Server API" since a cluster worker is both (it has a listening and exit event).
I chose the net.Server as the primary base, since the v0.6.x cluster module was build on child_process.fork and did clearly prevent a lot of extensibility to be added in the refactor. But to maintain some backward compatibility there would be a .process event, linking the server like object to a process.

It funny I remember having this discussion before, perhaps on IRC or in my head, who knows?

@isaacs
Copy link

isaacs commented Mar 3, 2013

@bnoordhuis @AndreasMadsen I have very little opinion on this, except that it'd be nice to not break backwards compatibility with 0.8. Maybe an alias?

@AndreasMadsen
Copy link
Member

If it should be renamed then do the following:

  • Alias .destroy() to .kill()
  • Deprecate .destroy()
  • Add optional signal argument to .kill()

Especially the signal argument seams important, since it will then look like the require('child_process').fork().kill method.

isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Mar 3, 2013
Fix nodejs#4133, bringing the cluster worker API more in line with the
child process API.
@isaacs isaacs closed this as completed in e428bb7 Mar 4, 2013
richardlau pushed a commit to ibmruntimes/node that referenced this issue Dec 3, 2015
Security Update

Notable items:

* build: Add support for Microsoft Visual Studio 2015
* npm: Upgrade to v1.4.29 from v1.4.28. A special one-off release as
  part of the strategy to get a version of npm into Node.js v0.10.x that
  works with the current registry
  (nodejs/Release#37). This version of npm prints
  out a banner each time it is run. The banner warns that the next
  standard release of Node.js v0.10.x will ship with a version of npm
  v2.
* openssl: Upgrade to 1.0.1q, containing fixes CVE-2015-3194
  "Certificate verify crash with missing PSS parameter", a potential
  denial-of-service vector for Node.js TLS servers; TLS clients are also
  impacted. Details are available at
  <http://openssl.org/news/secadv/20151203.txt>. (Ben Noordhuis)
  nodejs#4133

PR-URL: nodejs-private/node-private#15
gibfahn pushed a commit to ibmruntimes/node that referenced this issue Apr 1, 2016
Security Update

Notable items:

* http: Fix a bug where an HTTP socket may no longer have a socket but a
  pipelined request triggers a pause or resume, a potential
  denial-of-service vector. (Fedor Indutny)
* openssl: Upgrade to 1.0.1q, fixes CVE-2015-3194
  "Certificate verify crash with missing PSS parameter", a potential
  denial-of-service vector for Node.js TLS servers; TLS clients are also
  impacted. Details are available at
  <http://openssl.org/news/secadv/20151203.txt>. (Ben Noordhuis) nodejs#4133

PR-URL: nodejs-private/node-private#13
jBarz pushed a commit to ibmruntimes/node that referenced this issue Nov 4, 2016
Security Update

Notable items:

* http: Fix a bug where an HTTP socket may no longer have a socket but a
  pipelined request triggers a pause or resume, a potential
  denial-of-service vector. (Fedor Indutny)
* openssl: Upgrade to 1.0.1q, fixes CVE-2015-3194
  "Certificate verify crash with missing PSS parameter", a potential
  denial-of-service vector for Node.js TLS servers; TLS clients are also
  impacted. Details are available at
  <http://openssl.org/news/secadv/20151203.txt>. (Ben Noordhuis) nodejs#4133

PR-URL: https://github.com/nodejs/node-private/pull/13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants