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

Prevent multiple calls to Server.listen #6238

Closed
wants to merge 1 commit into from
Closed

Prevent multiple calls to Server.listen #6238

wants to merge 1 commit into from

Conversation

justsml
Copy link

@justsml justsml commented Apr 15, 2016

Checklist
  • tests and code linting passes
  • a test and/or benchmark is included
  • documentation is changed or added
  • the commit message follows commit guidelines
Affected core subsystem(s)

net

Description of change

Minimal attempt to resolve #6190

Added _listenHasBeenCalled as suggested by @trevnorris

justsml
justsml reviewed Apr 15, 2016
View reviewed changes
test/parallel/test-net-listen-multiple-calls.js Outdated
}, /cannot call "listen" multiple times/i);

process.on('exit', () => {
// Is this needed? assert.equal(server.address().port, 8080);
Copy link
Author

Choose a reason for hiding this comment

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

Is this necessary?

@mscdex mscdex added the net Issues and PRs related to the net subsystem. label Apr 15, 2016
thefourtheye
thefourtheye reviewed Apr 17, 2016
View reviewed changes
test/parallel/test-net-listen-multiple-calls.js
@@ -0,0 +1,17 @@
const assert = require('assert');
Copy link
Contributor

Choose a reason for hiding this comment

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

../common should be the first required line.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also run the test in strict mode

Copy link
Author

Choose a reason for hiding this comment

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

Thanks @thefourtheye - added ../common and strict mode.

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 18, 2016
@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

Conservatively marking this as a major due to the change in behavior but it's likely just fine as a minor. Feel free to downgrade if appropriate.

jasnell
jasnell reviewed Apr 18, 2016
View reviewed changes
lib/net.js Outdated
@@ -1300,6 +1300,11 @@ function listen(self, address, port, addressType, backlog, fd, exclusive) {
Server.prototype.listen = function() {
var self = this;

if (self._listenHasBeenCalled)
throw new Error('cannot call "listen" multiple times');
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps, 'listen() cannot be called multiple times.'

@justsml
Copy link
Author

justsml commented Apr 18, 2016

Integrated feedback from @jasnell @thefourtheye @estliberitas
Thx!

I am not sure on this: does anyone think this needs documentation? I'm wondering because of the semver-major tag... I know that's not necessarily the point of semver, I was just thinking this was 'minor' - and only affected accidentally 'working' edge-case/broken code.

@jasnell
Copy link
Member

jasnell commented Apr 18, 2016

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

@justsml
Copy link
Author

justsml commented Apr 19, 2016

I've run the tests on my Debian 7 & 8 systems, as well as on OSX... All pass.

@jasnell I'm trying to pull up the 'TAP Test Results' to see failure/details... but it is not coming up.
I am probably missing something, please help when you get a chance.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

Seeing this:

not ok 625 test-net-server-listen-remove-callback.js
# net.js:1305
#     throw new Error('listen() cannot be called multiple times.');
#     ^
# 
# Error: listen() cannot be called multiple times.
#     at Server.listen (net.js:1305:11)
#     at Server.<anonymous> (/Users/iojs/build/workspace/node-test-commit-osx/nodes/osx1010/test/parallel/test-net-server-listen-remove-callback.js:20:10)
#     at Server.g (events.js:271:16)
#     at emitNone (events.js:85:20)
#     at Server.emit (events.js:179:7)
#     at emitCloseNT (net.js:1527:8)
#     at _combinedTickCallback (internal/process/next_tick.js:71:11)
#     at process._tickCallback (internal/process/next_tick.js:98:9)
#     at Function.Module.runMain (module.js:566:11)
#     at startup (node.js:159:18)
# Closed, listeners: 0

@justsml
Copy link
Author

justsml commented Apr 19, 2016

@jasnell Am I not using assert.throws correctly?

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

It's in a different test ... test/parallel/test-net-server-listen-remove-callback.js

Do make -j8 test and it should come up.

@jasnell
Copy link
Member

jasnell commented Apr 19, 2016

Basically, you'll need to update any other tests that happen to be impacted by this change

@justsml
Copy link
Author

justsml commented Apr 24, 2016

I was hitting several fails on other tests, but I think I figured out a way to only require changes to 1 other test - essentially I just throw the error later.
I'm gonna finish testing and get it updated asap.

@rvagg rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:01
@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017
@jasnell
Copy link
Member

jasnell commented Mar 1, 2017

I believe this is no longer necessary. /cc @bnoordhuis
Closing for lack of further progress.

@jasnell jasnell closed this Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
net Issues and PRs related to the net subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

net: multiple listen() events fail silently
6 participants
@justsml @jasnell @estliberitas @evanlucas @thefourtheye @mscdex