Skip to content

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

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.

Sign up for GitHub

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

Merged
bnoordhuis merged 1 commit into from
Feb 25, 2016
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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
View file
Open in desktop
Original file line numberDiff line numberDiff 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);

@thefourtheyethefourtheyeFeb 22, 2016

Copy link
Copy Markdown
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?

@bnoordhuisbnoordhuisFeb 22, 2016

Copy link
Copy Markdown
MemberAuthor

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

@thefourtheyethefourtheyeFeb 22, 2016

Copy link
Copy Markdown
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 :-)

@thefourtheyethefourtheyeFeb 22, 2016

Copy link
Copy Markdown
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?

@bnoordhuisbnoordhuisFeb 22, 2016

Copy link
Copy Markdown
MemberAuthor

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

@thefourtheyethefourtheyeFeb 22, 2016

Copy link
Copy Markdown
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
View file
Open in desktop
Original file line numberDiff line numberDiff 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