FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

net: refactor overloaded argument handling by joyeecheung · Pull Request #11667 · nodejs/node · GitHub

/ node Public

net: refactor overloaded argument handling - #11667

Closed
joyeecheung wants to merge 5 commits into
nodejs:masterfrom
joyeecheung:net-args
ConversationCommits (5)ChecksFiles changed
Closed

net: refactor overloaded argument handling#11667
joyeecheung wants to merge 5 commits into
nodejs:masterfrom
joyeecheung:net-args

Conversation

joyeecheung commented Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member
  • Destructure arguments when it will always be converted into an array
  • Rename some args[i] for readability
  • Use Socket.prototype.connect.call instead of .apply when the number
    of arguments is certain(returned by normalizeArgs)
  • Make normalizeArgs return either [options, null] or [options, cb]
    (the second element won't be undefined anymore) and avoid OOB read
  • Refactor Server.prototype.listen, separate backlogFromArgs and
    options.backlog, avoid making options a handle, comment the
    overloading process

Pending benchmark results...

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

net

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Mar 3, 2017

joyeecheung commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-pull-request/6676/
Note: this PR is not supposed to change any existing behavior

joyeecheung commented Mar 3, 2017
View reviewed changes
Comment thread lib/net.js Outdated

joyeecheung Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The doc says

If the hostname is omitted, the server will accept connections on the unspecified IPv6 address (::) when IPv6 is available, or the unspecified IPv4 address (0.0.0.0) otherwise.

But looks like previously it just went straight to IPv4? (This PR doesn't change that)

sam-github Mar 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

this is reference to the comment, // Undefined host, listens on unspecified IPv4 address?

joyeecheung Mar 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@sam-github Oh looks like Server.prototype._listen2 would ignore addressType when address is null and try to bind the unspecified IPv6 address...nevermind then :)

mscdex reviewed Mar 3, 2017
View reviewed changes
Comment thread lib/net.js Outdated

mscdex Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I thought this was still slow (at least in some cases)? /cc @jasnell

joyeecheung Mar 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

AFAIK it is slower when you can skip converting it into array under some condition(and only when you call it with arguments that are named), but not when you are always converting it and heavily overloading it like this...

mscdex Mar 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I was thinking it more had to do with the number of arguments passed or something along those lines...

targos Mar 3, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The slowness does not only come from the rest parameter itself. Its presence disables Crankshaft for this function and it's possible that TurboFan generates slower code.

joyeecheung Mar 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Related: https://fhinkel.github.io/six-speed/#test-rest says rest parameters are faster, but our restparams-bench benchmark says otherwise...though none of them are testing overloaded arguments

mscdex reviewed Mar 3, 2017
View reviewed changes
Comment thread lib/net.js Outdated

mscdex Mar 3, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Unintentional whitespace change?

joyeecheung Mar 3, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed, thanks!

joyeecheung commented Mar 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

Previous CI failed in a bizarre way...(says "All checks have failed" but https://ci.nodejs.org/job/node-test-commit/8233/ is mostly green..?)

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

joyeecheung commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

Nothing with enough confidence shows up in the benchmark results..

Benchmark results 
                                                   improvement confidence    p.value
 net/net-c2s-cork.js dur=5 type="buf" len=1024         -2.31 %            0.26941557
 net/net-c2s-cork.js dur=5 type="buf" len=128           1.61 %            0.35423657
 net/net-c2s-cork.js dur=5 type="buf" len=16           -0.15 %            0.93907021
 net/net-c2s-cork.js dur=5 type="buf" len=32            3.33 %            0.08961369
 net/net-c2s-cork.js dur=5 type="buf" len=4             0.62 %            0.74934703
 net/net-c2s-cork.js dur=5 type="buf" len=512           1.27 %            0.66222617
 net/net-c2s-cork.js dur=5 type="buf" len=64            2.31 %            0.24750306
 net/net-c2s-cork.js dur=5 type="buf" len=8             1.79 %            0.31765058
 net/net-c2s.js dur=5 type="asc" len=102400            -2.65 %            0.34713637
 net/net-c2s.js dur=5 type="asc" len=16777216           1.85 %            0.70764228
 net/net-c2s.js dur=5 type="buf" len=102400             4.10 %            0.38920034
 net/net-c2s.js dur=5 type="buf" len=16777216          -5.24 %            0.15459963
 net/net-c2s.js dur=5 type="utf" len=102400            -2.31 %            0.53227401
 net/net-c2s.js dur=5 type="utf" len=16777216           0.28 %            0.94308128
 net/net-pipe.js dur=5 type="asc" len=102400            2.79 %            0.61437373
 net/net-pipe.js dur=5 type="asc" len=16777216          2.38 %            0.63068650
 net/net-pipe.js dur=5 type="buf" len=102400            2.66 %            0.60057916
 net/net-pipe.js dur=5 type="buf" len=16777216         -0.88 %            0.77870985
 net/net-pipe.js dur=5 type="utf" len=102400           -5.81 %            0.13508888
 net/net-pipe.js dur=5 type="utf" len=16777216          2.76 %            0.09846037
 net/net-s2c.js dur=5 type="asc" len=102400             1.83 %            0.49287261
 net/net-s2c.js dur=5 type="asc" len=16777216          -0.78 %            0.75951114
 net/net-s2c.js dur=5 type="buf" len=102400             1.77 %            0.37888441
 net/net-s2c.js dur=5 type="buf" len=16777216          -0.83 %            0.76237945
 net/net-s2c.js dur=5 type="utf" len=102400             2.04 %            0.25163787
 net/net-s2c.js dur=5 type="utf" len=16777216          -2.33 %            0.46584938
 net/tcp-raw-c2s.js dur=5 type="asc" len=102400         1.84 %            0.48664417
 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216       2.99 %            0.31863285
 net/tcp-raw-c2s.js dur=5 type="buf" len=102400        -1.23 %            0.43411749
 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216      -0.87 %            0.68639700
 net/tcp-raw-c2s.js dur=5 type="utf" len=102400         2.95 %            0.09170229
 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216       1.44 %            0.51893571
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400        1.00 %            0.86801283
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216     -1.41 %            0.52368250
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400        2.81 %            0.52655840
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216      1.43 %            0.55855929
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400       55.24 %            0.06649256
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     -0.47 %            0.84940769
 net/tcp-raw-s2c.js dur=5 type="asc" len=102400        -0.98 %            0.48191208
 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216       2.25 %            0.33763947
 net/tcp-raw-s2c.js dur=5 type="buf" len=102400        -1.83 %            0.28434154
 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216       0.89 %            0.39528692
 net/tcp-raw-s2c.js dur=5 type="utf" len=102400        -1.13 %            0.45419167
 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216       3.17 %            0.15172657

joyeecheung commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

This is causing some consistent errors on windows-fanned with vcbt2015(RUN_SUBSET = 2)..I'll split the commits and try to find out what is causing this

joyeecheung changed the title net: refactor overloaded argument handling [WIP]net: refactor overloaded argument handling Mar 3, 2017

joyeecheung commented Mar 3, 2017

Copy link
Copy Markdown
Member Author

Turns out it removed the case listen(TCP) before...reverted. Also reverted the rest params.
Should be green now. New CI: https://ci.nodejs.org/job/node-test-pull-request/6690/

joyeecheung changed the title [WIP]net: refactor overloaded argument handling net: refactor overloaded argument handling Mar 4, 2017

joyeecheung commented Mar 4, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author
Benchmark numbers 
                                                   improvement confidence     p.value
 net/net-c2s-cork.js dur=5 type="buf" len=1024          0.51 %            0.847875581
 net/net-c2s-cork.js dur=5 type="buf" len=128           1.64 %            0.546539268
 net/net-c2s-cork.js dur=5 type="buf" len=16            0.61 %            0.810465920
 net/net-c2s-cork.js dur=5 type="buf" len=32            0.32 %            0.878093841
 net/net-c2s-cork.js dur=5 type="buf" len=4             1.38 %            0.467924486
 net/net-c2s-cork.js dur=5 type="buf" len=512          -0.08 %            0.976736133
 net/net-c2s-cork.js dur=5 type="buf" len=64            4.72 %          * 0.040232528
 net/net-c2s-cork.js dur=5 type="buf" len=8             1.65 %            0.451122281
 net/net-c2s.js dur=5 type="asc" len=102400            -3.94 %            0.080389731
 net/net-c2s.js dur=5 type="asc" len=16777216           3.07 %            0.243184465
 net/net-c2s.js dur=5 type="buf" len=102400            -1.99 %            0.407039086
 net/net-c2s.js dur=5 type="buf" len=16777216           1.94 %            0.376930199
 net/net-c2s.js dur=5 type="utf" len=102400            -3.40 %            0.050772843
 net/net-c2s.js dur=5 type="utf" len=16777216          -0.66 %            0.688283137
 net/net-pipe.js dur=5 type="asc" len=102400            2.86 %            0.189891017
 net/net-pipe.js dur=5 type="asc" len=16777216          1.28 %            0.567203334
 net/net-pipe.js dur=5 type="buf" len=102400           -0.04 %            0.980100699
 net/net-pipe.js dur=5 type="buf" len=16777216         -1.60 %            0.495281133
 net/net-pipe.js dur=5 type="utf" len=102400            0.12 %            0.950469747
 net/net-pipe.js dur=5 type="utf" len=16777216         -0.76 %            0.617628652
 net/net-s2c.js dur=5 type="asc" len=102400            -2.91 %            0.109145558
 net/net-s2c.js dur=5 type="asc" len=16777216          -1.11 %            0.450574592
 net/net-s2c.js dur=5 type="buf" len=102400            -1.68 %            0.294086039
 net/net-s2c.js dur=5 type="buf" len=16777216           3.28 %            0.147678955
 net/net-s2c.js dur=5 type="utf" len=102400            -0.88 %            0.615214884
 net/net-s2c.js dur=5 type="utf" len=16777216          -0.72 %            0.709583085
 net/tcp-raw-c2s.js dur=5 type="asc" len=102400        -2.01 %            0.449092918
 net/tcp-raw-c2s.js dur=5 type="asc" len=16777216       0.96 %            0.663394878
 net/tcp-raw-c2s.js dur=5 type="buf" len=102400        -2.15 %            0.463581812
 net/tcp-raw-c2s.js dur=5 type="buf" len=16777216       0.55 %            0.849280563
 net/tcp-raw-c2s.js dur=5 type="utf" len=102400        -0.13 %            0.960543687
 net/tcp-raw-c2s.js dur=5 type="utf" len=16777216      -0.62 %            0.806655208
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400        3.42 %            0.595403945
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216    123.35 %         ** 0.005935044
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400       12.58 %          * 0.035927483
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216     41.86 %            0.104380751
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400       -2.46 %            0.782995053
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     18.08 %            0.308727856
 net/tcp-raw-s2c.js dur=5 type="asc" len=102400        -3.76 %            0.297974161
 net/tcp-raw-s2c.js dur=5 type="asc" len=16777216       2.32 %            0.441501849
 net/tcp-raw-s2c.js dur=5 type="buf" len=102400        -2.41 %            0.511049295
 net/tcp-raw-s2c.js dur=5 type="buf" len=16777216      -1.40 %            0.732423279
 net/tcp-raw-s2c.js dur=5 type="utf" len=102400        -4.65 %            0.105507183
 net/tcp-raw-s2c.js dur=5 type="utf" len=16777216      -1.56 %            0.462116407

The tcp-raw-pipe seems to make a difference, but running it again...

See numbers 
                                                   improvement confidence   p.value
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400       -1.02 %            0.9438923
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216    165.70 %            0.2828262
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400      -11.90 %            0.2488332
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216    -41.46 %            0.3789885
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400      -13.93 %            0.3436313
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     25.75 %            0.3271788

And run it master against master...

See numbers(wait wat) 
                                                   improvement confidence   p.value
 net/tcp-raw-pipe.js dur=5 type="asc" len=102400      -12.05 %            0.4330058
 net/tcp-raw-pipe.js dur=5 type="asc" len=16777216     58.87 %            0.4848385
 net/tcp-raw-pipe.js dur=5 type="buf" len=102400       -5.68 %            0.6042492
 net/tcp-raw-pipe.js dur=5 type="buf" len=16777216     15.40 %            0.4404852
 net/tcp-raw-pipe.js dur=5 type="utf" len=102400      -15.17 %            0.2378737
 net/tcp-raw-pipe.js dur=5 type="utf" len=16777216     20.97 %            0.4273859

¯\(ツ)/¯ I would interpret this as "no significant difference" and the first results as "coincidence".

CI is green. Need LTGMs..

joyeecheung mentioned this pull request Mar 4, 2017
Closed
3 tasks

gibfahn commented Mar 4, 2017

Copy link
Copy Markdown
Member

Based on onboarding-extras, cc/ @bnoordhuis, @indutny, @nodejs/streams

mcollina approved these changes Mar 4, 2017
View reviewed changes

mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM pending CITGM run

jasnell reviewed Mar 4, 2017
View reviewed changes
Comment thread lib/net.js Outdated

jasnell Mar 4, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

For the TODO, can you please use the TODO (@joyeecheung): syntax?

joyeecheung commented Mar 5, 2017

Copy link
Copy Markdown
Member Author

Added handle after TODO @jasnell :)

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/621/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/622/

gibfahn reviewed Mar 5, 2017
View reviewed changes
Comment thread lib/net.js Outdated

gibfahn Mar 5, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Super-nit, but none of the other TODOs in the codebase have an @ in them, the most standard style I can find is:

// TODO(joyeecheung): use ...

joyeecheung commented Mar 5, 2017 •
edited
Loading

Copy link
Copy Markdown
Member Author

@gibfahn OK, fixed. BTW this convention can be mentioned in STYLE_GUIDE.md?

CITGM is (27 failures / ±0). Is this ready to land after the 72-hour wait?
New CI: https://ci.nodejs.org/job/node-test-pull-request/6709/

gibfahn commented Mar 5, 2017

Copy link
Copy Markdown
Member

@joyeecheung Looks like the only thing that failed in citgm on this PR but not in master was serialport on Ubuntu 14.04, failure was:

  3 failing
   1) SerialPortBinding #list returns an array:
      Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.
   
   2) SerialPortBinding #list has objects with undefined when there is no data:
      Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.
   
   3) SerialPort light integration .list:
      Error: timeout of 2000ms exceeded. Ensure the done() callback is being called in this test.

I reran just serialport on this PR (failed) and on master (passed), so the failure does seem to be consistent.

gibfahn commented Mar 5, 2017

Copy link
Copy Markdown
Member

cc/ @nodejs/citgm

joyeecheung commented Mar 5, 2017

Copy link
Copy Markdown
Member Author

@gibfahn Ooops, sorry for not checking it more carefully..

Rebased against the latest master and tried again to be certain...If it still fails, I will try different commits to see which one is causing this.
latest master, PR

net: make normalizeArgs return tuples
ba793bc 
Make normalizeArgs return either [options, null] or [options, cb]
(the second element won't be undefined anymore) and avoid OOB read
net: replace apply with call
a8d0c24 
Use Socket.prototype.connect.call instead of .apply when the number
of arguments is certain(returned by normalizeArgs).
net: rename some args[i] for readability
1ea81d5
net: refactor Server.prototype.listen
ce3fc05 
* Separate backlogFromArgs and options.backlog
* Comment the overloading process
net: refactor server.listen flow control
bcd4ff7
joyeecheung added a commit that referenced this pull request Mar 8, 2017
net: refactor overloaded argument handling
53f3869 
* Make normalizeArgs return either [options, null] or [options, cb]
  (the second element won't be undefined anymore) and avoid OOB read
* Use Socket.prototype.connect.call instead of .apply when the number
  of arguments is certain(returned by normalizeArgs).
* Rename some args[i] for readability
* Refactor Server.prototype.listen, separate backlogFromArgs and
  options.backlog, comment the overloading process, refactor control
  flow

PR-URL: #11667
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung mentioned this pull request Mar 8, 2017
Closed
3 tasks
evanlucas pushed a commit that referenced this pull request Mar 8, 2017
net: refactor overloaded argument handling
b56e851 
* Make normalizeArgs return either [options, null] or [options, cb]
  (the second element won't be undefined anymore) and avoid OOB read
* Use Socket.prototype.connect.call instead of .apply when the number
  of arguments is certain(returned by normalizeArgs).
* Rename some args[i] for readability
* Refactor Server.prototype.listen, separate backlogFromArgs and
  options.backlog, comment the overloading process, refactor control
  flow

PR-URL: #11667
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
evanlucas mentioned this pull request Mar 8, 2017
Merged
vhain mentioned this pull request Mar 9, 2017
Closed
bajtos mentioned this pull request Mar 9, 2017
Merged
watilde added a commit to watilde/node that referenced this pull request Mar 12, 2017
net: remove an unused internal module assertPort
23587b6 
A module `assertPort` in `lib/internal/net.js` is not used anymore.

Refs: nodejs#11667
watilde mentioned this pull request Mar 12, 2017
Closed
2 tasks
watilde added a commit that referenced this pull request Mar 14, 2017
net: remove an unused internal module assertPort
879d666 
A module `assertPort` in `lib/internal/net.js` is not used anymore.

Refs: #11667
PR-URL: #11812
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 14, 2017
test: fail when child dies in fork-net
c9bc3e5 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: #11684
Ref: #11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Mar 20, 2017
test: fail when child dies in fork-net
5f6025b 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: nodejs#11684
Ref: nodejs#11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
net: refactor overloaded argument handling
f5a59e0 
* Make normalizeArgs return either [options, null] or [options, cb]
  (the second element won't be undefined anymore) and avoid OOB read
* Use Socket.prototype.connect.call instead of .apply when the number
  of arguments is certain(returned by normalizeArgs).
* Rename some args[i] for readability
* Refactor Server.prototype.listen, separate backlogFromArgs and
  options.backlog, comment the overloading process, refactor control
  flow

PR-URL: nodejs#11667
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
net: remove an unused internal module assertPort
7f8cd16 
A module `assertPort` in `lib/internal/net.js` is not used anymore.

Refs: nodejs#11667
PR-URL: nodejs#11812
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
jungx098 pushed a commit to jungx098/node that referenced this pull request Mar 21, 2017
test: fail when child dies in fork-net
1ba472d 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: nodejs#11684
Ref: nodejs#11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

MylesBorins commented Apr 17, 2017

Copy link
Copy Markdown
Contributor

is this something we want to backport to v6.x?

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
test: fail when child dies in fork-net
0d4d67a 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: #11684
Ref: #11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
test: fail when child dies in fork-net
4d916da 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: #11684
Ref: #11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
mcollina mentioned this pull request May 2, 2017
Closed
3 tasks
watilde added a commit to watilde/node that referenced this pull request May 5, 2017
net: remove an unused internal module assertPort
acb77f3 
A module `assertPort` in `lib/internal/net.js` is not used anymore.

Refs: nodejs#11667
PR-URL: nodejs#11812
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

MylesBorins commented May 8, 2017

Copy link
Copy Markdown
Contributor

ping

joyeecheung self-assigned this May 9, 2017

joyeecheung commented May 9, 2017

Copy link
Copy Markdown
Member Author

Ah missed this one, sorry. This should land with #11762, and probably #11847. I will try to see if it's possible

joyeecheung commented May 10, 2017

Copy link
Copy Markdown
Member Author

So this PR depends on #4039 which is a 7.x semver-major. Added don't land label

watilde added a commit that referenced this pull request May 12, 2017
net: remove an unused internal module assertPort
33ea7a2 
A module `assertPort` in `lib/internal/net.js` is not used anymore.

Refs: #11667
PR-URL: #11812
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
test: fail when child dies in fork-net
d070f81 
Previously when the child dies with errors in this test, the parent
will just hang and timeout, the errors in the child would be
swallowed. This makes it fail so at least there is more information
about why this test fails.

Also removes the unnecessary child.kill() call.

PR-URL: nodejs/node#11684
Ref: nodejs/node#11667
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
joyeecheung mentioned this pull request Oct 17, 2017
Closed
2 tasks
joyeecheung removed their assignment Oct 17, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
jasnell jasnell approved these changes
+4 more reviewers
sam-github sam-github left review comments
mscdex mscdex left review comments
targos targos left review comments
gibfahn gibfahn left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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.

9 participants

Back | FazBrowse Home | New Git URL