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

doc,lib: correct child.send() return value docs #3518

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Oct 25, 2015

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.

child.send() returns undefined, contrary to the docs. Update docs and
remove related dead code.
@Trott
Copy link
Member Author

Trott commented Oct 25, 2015

@mscdex mscdex added child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations. labels Oct 25, 2015
@@ -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);
Copy link
Member

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.

Copy link
Member Author

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

@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

This would be a semver-minor at least if this lands which takes it out of the v4.x queue...

@Trott
Copy link
Member Author

Trott commented Oct 28, 2015

@jasnell This does not change the behavior of Node. That return statement that is removed is basically dead code. The return value (from _send()) is unused and discarded by send() and the end user never sees it. Node/iojs have been returning undefined from send() in all cases since at least iojs 1.0.

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!

@jasnell
Copy link
Member

jasnell commented Oct 28, 2015

@Trott ... TISC ;-) Ok, I had missed that part. I'll track #3516 and see how that evolves. It's still possible that this could hit in v4.x at some point but for now let's hold off.

@Trott
Copy link
Member Author

Trott commented Oct 28, 2015

Closed in favor of #3516

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
child_process Issues and PRs related to the child_process subsystem. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants