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

doc issue: socket server 'error' event: 'close' is not called as described in doc. #9710

Closed
jjqq2013 opened this issue Nov 20, 2016 · 4 comments
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem.

Comments

@jjqq2013
Copy link
Contributor

  • Version:v7.1.0
  • Platform:Darwin p14010112-no-MacBook-Pro.local 15.6.0 Darwin Kernel Version 15.6.0: Thu Jun 23 18:25:34 PDT 2016; root:xnu-3248.60.10~1/RELEASE_X86_64 x86_64
  • Subsystem:

I am curious that should i call server.close in server.on('error'...) handler?

According to the document about net.Server 'error' event

Event: 'error'#
<Error>
Emitted when an error occurs. The 'close' event will be called directly following this event. See example in discussion of server.listen.

But in fact, the 'close' event does not happen after error.

This is my test.js which try to listen a privileged port 80, cause an EACCESS error and exit.

require('net').createServer({allowHalfOpen: false}, function(stream) {
  //
}).listen({host: 'localhost', port: 80}, function () {
  //
}).on('error', function(e) {
  console.log(e.message);
  setTimeout(function () {
    //
  },1000)
}).on('close', function () {
  console.log('closed')
});

To ensure the close event can be caught before process exit, i set up a 1 second timer so process will keep alive in 1 second.

But still, the close event does not happen.

If i add this.close() in error handler, then the 'close' does happen.

According to another doc about server.listen, obviously node.js does not call close on error, at least for EADDRINUSE.

server.on('error', (e) => {
  if (e.code == 'EADDRINUSE') {
    console.log('Address in use, retrying...');
    setTimeout(() => {
      server.close();                       //see here please. If it is auto closed, why need this?
      server.listen(PORT, HOST);
    }, 1000);
  }
});

So the document is contradictory, it should be noted that 'close' will not be called automatically.

Regards.

@italoacasas italoacasas added doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem. labels Nov 20, 2016
@mscdex
Copy link
Contributor

mscdex commented Nov 20, 2016

It may have just been copied from the net.Socket documentation, where 'close' is always emitted (after 'error' and/or 'end').

@krydos
Copy link
Contributor

krydos commented Feb 2, 2017

@mscdex Is it ok to just remove everything except Emitted when an error occurs.?

If yes I can prepare pull request. If no, I still can prepare pull request with something else :)

@jjqq2013
Copy link
Contributor Author

jjqq2013 commented Feb 3, 2017

@krydos no, i think the doc should insert a word "not"

The 'close' event will be called directly following this event

=>

The 'close' event will NOT be called directly following this event

@jjqq2013
Copy link
Contributor Author

jjqq2013 commented Feb 3, 2017

@krydos i'v made a pull request.

jjqq2013 added a commit to sjitech/node that referenced this issue Feb 17, 2017
addaleax pushed a commit that referenced this issue Feb 22, 2017
Fixes: #9710
PR-URL: #11136
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
jasnell pushed a commit that referenced this issue Mar 7, 2017
Fixes: #9710
PR-URL: #11136
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
jasnell pushed a commit that referenced this issue Mar 7, 2017
Fixes: #9710
PR-URL: #11136
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Fixes: #9710
PR-URL: #11136
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 9, 2017
Fixes: #9710
PR-URL: #11136
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Sam Roberts <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants