cluster: emit worker as first 'message' event arg #5361

bnoordhuis · 2016-02-22T11:41:54Z

It's documented as such but didn't actually behave that way.

Bug introduced in commit 66fc8ca ("cluster: emit 'message' event on
cluster master"), which is the commit that introduced the event.

Fixes: #5126

R=@sam-github

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

thefourtheye · 2016-02-22T12:14:37Z

lib/cluster.js

-      cluster.emit('message', message, handle)
-    );
+    worker.on('message', function(message, handle) {
+      cluster.emit('message', this, message, handle);


Probably I am missing something. When the message event is emitted by cluster, the this will be cluster, not the worker object, right?

The event is emitted on the worker object, the listener simply re-emits it on the cluster object (hence why it was changed from an arrow function to a normal function - no lexical this.)

@bnoordhuis Thanks! Got it now :-)

Wouldn't it be better if we say worker instead of this?

I can change it if you think it's clearer. The way it's written now saves a few bytes of memory and reduces GC strain infinitesimally (one fewer closure context variable to traverse.)

Even I was thinking about the closure variable access. But then every time someone goes through the code, they would have to spend time on this to really understand this. Maybe that's just me.

thefourtheye · 2016-02-22T13:42:22Z

LGTM

cjihrig · 2016-02-22T13:51:11Z

LGTM

ofrobots · 2016-02-22T15:28:55Z

doc/api/cluster.markdown


 Emitted when any worker receives a message.

 See [child_process event: 'message'][].

+Until Node.js v6.0, this event emitted only the message and the handle,


Nit: this wording ('Until') makes it ambiguous what the behaviour of v6.0 is. Use 'Before Node.js v6.0' perhaps?

It's documented as such but didn't actually behave that way. Bug introduced in commit 66fc8ca ("cluster: emit 'message' event on cluster master"), which is the commit that introduced the event. Fixes: nodejs#5126 PR-URL: nodejs#5361 Reviewed-By: Ali Ijaz Sheikh <ofrobots@google.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

bnoordhuis · 2016-02-25T11:49:17Z

Landed in 66f4586 with Ali's feedback incorporated. Thanks for the review everyone.

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

The following significant (semver-major) changes have been made since the previous Node v5.0.0 release. * Buffer * New Buffer constructors have been added [#4682](#4682) * Previously deprecated Buffer APIs are removed [#5048](#5048), [#4594](#4594) * Improved error handling [#4514](#4514) * Cluster * Worker emitted as first argument in 'message' event [#5361](#5361). * Crypto * Improved error handling [#3100](#3100), [#5611](#5611) * Simplified Certificate class bindings [#5382](#5382) * Improved control over FIPS mode [#5181](#5181) * pbkdf2 digest overloading is deprecated [#4047](#4047) * Dependencies * Reintroduce shared c-ares build support [#5775](#5775). * V8 updated to 5.0.71.31 [#6111](#6111). * DNS * Add resolvePtr API to query plain DNS PTR records [#4921](#4921). * Domains * Clear stack when no error handler [#4659](#4659). * File System * The `fs.realpath()` and `fs.realpathSync()` methods have been updated to use a more efficient libuv implementation. This change includes the removal of the `cache` argument and the method can throw new errors [#3594](#3594) * FS apis can now accept and return paths as Buffers [#5616](#5616). * Error handling and type checking improvements [#5616](#5616), [#5590](#5590), [#4518](#4518), [#3917](#3917). * fs.read's string interface is deprecated [#4525](#4525) * HTTP * 'clientError' can now be used to return custom errors from an HTTP server [#4557](#4557). * Modules * Current directory is now prioritized for local lookups [#5689](#5689) * Symbolic links are preserved when requiring modules [#5950](#5950) * Net * DNS hints no longer implicitly set [#6021](#6021). * Improved error handling and type checking [#5981](#5981), [#5733](#5733), [#2904](#2904) * OS X * MACOSX_DEPLOYMENT_TARGET has been bumped up to 10.7 [#6402](#6402). * Path * Improved type checking [#5348](#5348). * Process * Introduce process warnings API [#4782](#4782). * Throw exception when non-function passed to nextTick [#3860](#3860). * Readline * Emit key info unconditionally [#6024](#6024) * REPL * Assignment to `_` will emit a warning. [#5535](#5535) * Timers * Fail early when callback is not a function [#4362](#4362) * TLS * Rename 'clientError' to 'tlsClientError' [#4557](#4557) * SHA1 used for sessionIdContext [#3866](#3866) * TTY * Previously deprecated setRawMode wrapper is removed [#2528](#2528). * Util * Changes to Error object formatting [#4582](#4582). * Windows * Windows XP and Vista are no longer supported [#5167](#5167), [#5167](#5167).

bnoordhuis added cluster Issues and PRs related to the cluster subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. labels Feb 22, 2016

bnoordhuis mentioned this pull request Feb 22, 2016

Either Cluster API doc or Cluster event 'message' argument is wrong #5126

Closed

thefourtheye reviewed Feb 22, 2016
View reviewed changes

ofrobots reviewed Feb 22, 2016
View reviewed changes

bnoordhuis force-pushed the fix5126 branch 2 times, most recently from 83773b0 to 119f124 Compare February 25, 2016 11:44

bnoordhuis force-pushed the fix5126 branch from 119f124 to 66f4586 Compare February 25, 2016 11:48

bnoordhuis closed this Feb 25, 2016

bnoordhuis deleted the fix5126 branch February 25, 2016 11:48

bnoordhuis merged commit 66f4586 into nodejs:master Feb 25, 2016

jasnell mentioned this pull request Mar 17, 2016

Planning for v6 #5766

Closed

jasnell mentioned this pull request Apr 19, 2016

What is new in v6? #6264

Closed

jasnell mentioned this pull request Apr 26, 2016

2016-04-26, Version 6.0.0 (Current) Release #6383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cluster: emit worker as first 'message' event arg #5361

cluster: emit worker as first 'message' event arg #5361

bnoordhuis commented Feb 22, 2016

thefourtheye Feb 22, 2016

bnoordhuis Feb 22, 2016

thefourtheye Feb 22, 2016

thefourtheye Feb 22, 2016

bnoordhuis Feb 22, 2016

thefourtheye Feb 22, 2016

thefourtheye commented Feb 22, 2016

cjihrig commented Feb 22, 2016

ofrobots Feb 22, 2016

bnoordhuis commented Feb 25, 2016

cluster: emit worker as first 'message' event arg #5361

cluster: emit worker as first 'message' event arg #5361

Conversation

bnoordhuis commented Feb 22, 2016

thefourtheye Feb 22, 2016

Choose a reason for hiding this comment

bnoordhuis Feb 22, 2016

Choose a reason for hiding this comment

thefourtheye Feb 22, 2016

Choose a reason for hiding this comment

thefourtheye Feb 22, 2016

Choose a reason for hiding this comment

bnoordhuis Feb 22, 2016

Choose a reason for hiding this comment

thefourtheye Feb 22, 2016

Choose a reason for hiding this comment

thefourtheye commented Feb 22, 2016

cjihrig commented Feb 22, 2016

ofrobots Feb 22, 2016

Choose a reason for hiding this comment

bnoordhuis commented Feb 25, 2016