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

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

Closed
bennosom opened this issue Feb 6, 2016 · 5 comments
Closed
Labels
cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations.

Comments

@bennosom
Copy link

bennosom commented Feb 6, 2016

https://nodejs.org/dist/latest-v5.x/docs/api/cluster.html#cluster_event_message_1
Cluster event 'message' has two arguments: worker object and message? Really? I will only get one argument - the message.

Env: Windows, 64bit, v5.5.0

Would be nice if there is a worker object, because it makes life easier...

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations. labels Feb 6, 2016
@mscdex
Copy link
Contributor

mscdex commented Feb 6, 2016

@bennosom That's for the master. The worker process only has a message argument.

@bennosom
Copy link
Author

bennosom commented Feb 7, 2016

Yes, I agree with you. The master should have two arguments, but it has not:

var cluster = require('cluster');
if (cluster.isMaster) {
    cluster.fork();
    cluster.on('message', (worker, message) => {
        console.log(worker, message); // <-- prints: hello undefined
    });
} else {
    process.send('hello');
}

I would expect the worker object as first argument as mentioned in the docs (see https://nodejs.org/dist/latest-v5.x/docs/api/cluster.html#cluster_event_message_1).

@bnoordhuis
Copy link
Member

The documentation is at odds with the test coverage here.

@sam-github Is that an oversight in commit 66fc8ca? The intent was to emit the worker as the first argument?

@sam-github
Copy link
Contributor

I'm sorry, yes, it was an oversight, the intention was to emit the worker as the first argument, consistent with the other events on master.

@bnoordhuis
Copy link
Member

#5361

bnoordhuis added a commit to bnoordhuis/io.js that referenced this issue Feb 25, 2016
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

No branches or pull requests

4 participants