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.

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

Jump to bottom

net: honor default values in Socket constructor #19971

Closed
wants to merge 1 commit into from
Closed

net: honor default values in Socket constructor #19971

wants to merge 1 commit into from

Conversation

santigimeno
Copy link
Member

@santigimeno santigimeno commented Apr 12, 2018 •
edited by lpinca
Loading

Specifically readable and writable that default to false.

Fixes: libuv/libuv#1794

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added child_process Issues and PRs related to the child_process subsystem. net Issues and PRs related to the net subsystem. labels Apr 12, 2018
@lpinca
Copy link
Member

lpinca commented Apr 12, 2018 •
edited
Loading

Is this

node/lib/net.js

Line 288 in 5a8fcd0

this.readable = this.writable = false;

still needed with this change?

@mscdex
Copy link
Contributor

mscdex commented Apr 12, 2018

Can a test be included for this?

@lpinca
Copy link
Member

lpinca commented Apr 12, 2018

This also changes the default value for readable and writable from true to false when using only the fd option.

const socket = new net.Socket(fd);
assert(socket.readable);
assert(socket.writable);

@santigimeno
Copy link
Member Author

this.readable = this.writable = false;

Good catch. I'll remove that line.

Can a test be included for this?

Will do

This also changes the default value for readable and writable from true to false when using only the fd option.

Yes, isn't that what the documentation implies?

@BridgeAR BridgeAR added the wip Issues and PRs that are still a work in progress. label Apr 12, 2018
@lpinca
Copy link
Member

lpinca commented Apr 12, 2018

Yes it seems consistent with the docs but not with the code, not sure which one is correct :)

addaleax
addaleax reviewed Apr 12, 2018
View reviewed changes
lib/internal/wrap_js_stream.js Outdated
@@ -72,6 +72,8 @@ class JSStreamWrap extends Socket {
this.stream = stream;
this[kCurrentWriteRequest] = null;
this[kCurrentShutdownRequest] = null;
this.readable = true;
this.writable = true;
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought, but I think it might be more correct to forward these values from stream itself?

@santigimeno
Copy link
Member Author

Updated with comments addressed. PTAL. Thanks

@santigimeno
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/14226/

@cjihrig
Copy link
Contributor

cjihrig commented Apr 13, 2018

CI has a lot of red.

@santigimeno
Copy link
Member Author

Yes, I have to fix the test for Windows. The other failures seem unrelated.

@santigimeno
net: honor default values in Socket constructor
485bea9 
Specifically `readable` and `writable` that default to `false`.

Fixes: libuv/libuv#1794
@santigimeno
Copy link
Member Author

Added a fix for the test. New CI: https://ci.nodejs.org/job/node-test-pull-request/14400/

@santigimeno
Copy link
Member Author

CI looks good now.

lpinca
lpinca approved these changes Apr 21, 2018
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM. I'm only a little worried about the behavior change described in #19971 (comment).

Should we consider it a bug fix?

@lpinca lpinca added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed wip Issues and PRs that are still a work in progress. labels Apr 21, 2018
@apapirovski
Copy link
Member

Might as well run CitGM if we have concerns: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1380/

@BridgeAR
Copy link
Member

BridgeAR commented Apr 23, 2018 •
edited
Loading

Landed in 67e2a15 🎉

@BridgeAR BridgeAR closed this Apr 23, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 23, 2018
@santigimeno @BridgeAR
net: honor default values in Socket constructor
67e2a15 
Specifically `readable` and `writable` that default to `false`.

PR-URL: nodejs#19971
Fixes: libuv/libuv#1794
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@lpinca
Copy link
Member

lpinca commented Apr 23, 2018

Are we ok with this being semver-patch?

jasnell pushed a commit that referenced this pull request Apr 23, 2018
@santigimeno @jasnell
net: honor default values in Socket constructor
a342cd6 
Specifically `readable` and `writable` that default to `false`.

PR-URL: #19971
Fixes: libuv/libuv#1794
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
@MylesBorins MylesBorins mentioned this pull request Nov 5, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax left review comments

@apapirovski apapirovski apapirovski approved these changes

@lpinca lpinca lpinca approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. child_process Issues and PRs related to the child_process subsystem. net Issues and PRs related to the net subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Nodejs net.Socket working code stopped working with libuv 1.20 update
8 participants
@santigimeno @lpinca @mscdex @cjihrig @apapirovski @BridgeAR @addaleax @nodejs-github-bot