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

net: net.Server.listen() avoid operations on null when fail #23920

Closed
wants to merge 2 commits into from

Conversation

oyyd
Copy link
Contributor

@oyyd oyyd commented Oct 27, 2018

When net.Server fails to create a new handle, an error shall be emitted in the next tick. Therefore, we make net.Server.listen() directly return to avoid following operations on null this._handle.

Fixes: #23917

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

When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: nodejs#23917
@nodejs-github-bot nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Oct 27, 2018
@danbev
Copy link
Contributor

danbev commented Oct 30, 2018

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

Tests failed seems unrelated.

@richardlau
Copy link
Member

Tests failed seems unrelated.

No, the test failures on Windows are definitely related:

e.g. https://ci.nodejs.org/job/node-test-binary-windows/21115/COMPILED_BY=vs2017,RUNNER=win10,RUN_SUBSET=0/console

01:10:09 not ok 339 parallel/test-net-server-listen-path
01:10:09   ---
01:10:09   duration_ms: 0.252
01:10:09   severity: fail
01:10:09   exitcode: 1
01:10:09   stack: |-
01:10:09     assert.js:86
01:10:09       throw new AssertionError(obj);
01:10:09       ^
01:10:09     
01:10:09     AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
01:10:09     + actual - expected
01:10:09     
01:10:09     + 'EACCES'
01:10:09     - 'EADDRINUSE'
01:10:09          ^
01:10:09         at Server.srv.on.common.mustCall (c:\workspace\node-test-binary-windows\test\parallel\test-net-server-listen-path.js:82:12)
01:10:09         at Server.<anonymous> (c:\workspace\node-test-binary-windows\test\common\index.js:340:15)
01:10:09         at Server.emit (events.js:182:13)
01:10:09         at emitErrorNT (net.js:1317:8)
01:10:09         at internalTickCallback (internal/process/next_tick.js:72:19)
01:10:09         at process._tickCallback (internal/process/next_tick.js:47:5)
01:10:09         at Function.Module.runMain (internal/modules/cjs/loader.js:763:11)
01:10:09         at startup (internal/bootstrap/node.js:308:19)
01:10:09         at bootstrapNodeJSCore (internal/bootstrap/node.js:878:3)
01:10:09   ...

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

@richardlau Yes. Sorry for the negligence, will look into this.

@oyyd
Copy link
Contributor Author

oyyd commented Oct 30, 2018

Although I don't have a windows desktop on hand until the day after tomorrow, I have updated the test to make it emit an "EADDRINUSE" error explicitly. Hopefully it will work on windows.

@richardlau Thanks again for the reminding.

@richardlau
Copy link
Member

@Trott
Copy link
Member

Trott commented Nov 6, 2018

Landed in 3458e65

@Trott Trott closed this Nov 6, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 6, 2018
When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: nodejs#23917

PR-URL: nodejs#23920
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
targos pushed a commit that referenced this pull request Nov 6, 2018
When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: #23917

PR-URL: #23920
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
codebytere pushed a commit that referenced this pull request Nov 29, 2018
When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: #23917

PR-URL: #23920
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: #23917

PR-URL: #23920
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
@codebytere codebytere mentioned this pull request Nov 29, 2018
MylesBorins pushed a commit that referenced this pull request Dec 3, 2018
When `net.Server` fails to create a new handle, an error shall be
emitted in the next tick. Therefore, we make `net.Server.listen()`
directly  return to avoid following operations on `null`
`this._handle`.

Fixes: #23917

PR-URL: #23920
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Daniel Bevenius <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing error when listening with domain socket on already-existing path
6 participants