-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
doc,lib: correct child.send() return value docs #3518
Conversation
child.send() returns undefined, contrary to the docs. Update docs and remove related dead code.
@@ -610,9 +610,6 @@ function setupChannel(target, channel) { | |||
this.emit('error', ex); // FIXME(bnoordhuis) Defer to next tick. | |||
} | |||
} | |||
|
|||
/* If the master is > 2 read() calls behind, please stop sending. */ | |||
return channel.writeQueueSize < (65536 * 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not a good change. I remember this was introduced deliberately because unbound queue growth was a real problem with some applications.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's currently an unused value returned from _send()
and swallowed by send()
and has been since 0.12.0. The last time end users were able to get a boolean returned from send()
was 0.10.40. I'm not arguing that you're wrong. I'm just providing additional info/context in case that changes your estimation of the code's importance. (I'm guessing it does not and I certainly value your judgment on it more than my own. You have internalized a ton of historical context I am oblivious to.)
This would be a semver-minor at least if this lands which takes it out of the v4.x queue... |
@jasnell This does not change the behavior of Node. That return statement that is removed is basically dead code. The return value (from EDIT: That said, I'm fine with it not landing in LTS. And it looks like this isn't going to land anyway and that @bnoordhuis (and probably others?) favor #3516 (which changes the code to use that value) over it anyway. All of which is totally A-OK by me! |
Closed in favor of #3516 |
child.send() returns undefined, contrary to the docs. Update docs and
remove related dead code.
This is an alternative to #3516. That PR updates the behavior of the code to reflect the docs. This updates the docs to reflect the behavior of the code.