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

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

Merged
merged 1 commit into from
Feb 25, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions doc/api/cluster.markdown
Original file line number Diff line number Diff line change
Expand Up @@ -487,11 +487,29 @@ The `addressType` is one of:

* `worker` {cluster.Worker}
* `message` {Object}
* `handle` {undefined|Object}

Emitted when any worker receives a message.

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

Before Node.js v6.0, this event emitted only the message and the handle,
but not the worker object, contrary to what the documentation stated.

If you need to support older versions and don't need the worker object,
you can work around the discrepancy by checking the number of arguments:

```js
cluster.on('message', function(worker, message, handle) {
if (arguments.length === 2) {
handle = message;
message = worker;
worker = undefined;
}
// ...
});
```

## Event: 'online'

* `worker` {cluster.Worker}
Expand Down
6 changes: 3 additions & 3 deletions lib/cluster.js
Original file line number Diff line number Diff line change
Expand Up @@ -341,9 +341,9 @@ function masterInit() {
process: workerProcess
});

worker.on('message', (message, handle) =>
cluster.emit('message', message, handle)
);
worker.on('message', function(message, handle) {
cluster.emit('message', this, message, handle);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bnoordhuis Thanks! Got it now :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

});

worker.process.once('exit', function(exitCode, signalCode) {
/*
Expand Down
3 changes: 2 additions & 1 deletion test/parallel/test-cluster-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,8 @@ else if (cluster.isMaster) {
worker.on('message', function(message) {
check('master', message === 'message from worker');
});
cluster.on('message', function(message) {
cluster.on('message', function(worker_, message) {
assert.strictEqual(worker_, worker);
check('global', message === 'message from worker');
});

Expand Down