Skip to content

net: honor default values in Socket constructor#19971

Closed
santigimeno wants to merge 1 commit intonodejs:masterfrom
santigimeno:honor_readable_writable
Closed

net: honor default values in Socket constructor#19971
santigimeno wants to merge 1 commit intonodejs:masterfrom
santigimeno:honor_readable_writable

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

santigimeno commented Apr 12, 2018

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
Copy link
Member

@addaleax addaleax Apr 12, 2018

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 santigimeno force-pushed the honor_readable_writable branch from 4a21f02 to e4fdba2 Compare April 12, 2018 14:53
@santigimeno
Copy link
Member Author

santigimeno commented Apr 12, 2018

Updated with comments addressed. PTAL. Thanks

@santigimeno
Copy link
Member Author

santigimeno commented Apr 12, 2018

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

santigimeno commented Apr 13, 2018

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 santigimeno force-pushed the honor_readable_writable branch from e4fdba2 to 485bea9 Compare April 20, 2018 14:26
@santigimeno
Copy link
Member Author

santigimeno commented Apr 20, 2018

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

@santigimeno
Copy link
Member Author

santigimeno commented Apr 20, 2018

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
Contributor

apapirovski commented Apr 21, 2018

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

@lpinca lpinca lpinca approved these changes

+1 more reviewer

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

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