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

lib: make sure close the net server #51929

Merged

Conversation

theanarkh
Copy link
Contributor

If perform asynchronous operations in the listen function of net.js (execute dns.lookup or ask the main process to create a server), the server will not close properly.

The example is as follows.

// The "closed" TCP server will keep the process because 
// after calling close function, a TCP handle is created in the callback of `dns.lookup`.
const net = require('net');
net.createServer().listen(9999, 'localhost').close();

See

node/lib/net.js

Line 2092 in c08b797

function lookupAndListen(self, port, address, backlog, exclusive, flags) {
.

  • 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
Copy link
Collaborator

Review requested:

  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. net Issues and PRs related to the net subsystem. labels Feb 29, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 29, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 29, 2024
@nodejs-github-bot
Copy link
Collaborator

@mcollina mcollina added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels Mar 1, 2024
@lpinca
Copy link
Member

lpinca commented Mar 1, 2024

Does it work if the user does something like this?

net.createServer().listen(9999, 'localhost').close().listen(8888, 'localhost');

@theanarkh
Copy link
Contributor Author

Yes, close make the callback invalid of the first listen and the second listen executes normally.
I tested it locally, it ended up listening on port 8888 and just emit listening event once(before this PR, it will emit listening event twice).

@marco-ippolito marco-ippolito added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 3, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 3, 2024
@nodejs-github-bot nodejs-github-bot merged commit 29ec7e9 into nodejs:main Mar 3, 2024
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 29ec7e9

targos pushed a commit that referenced this pull request Mar 7, 2024
PR-URL: #51929
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@targos targos mentioned this pull request Mar 7, 2024
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51929
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
PR-URL: #51929
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
rdw-msft pushed a commit to rdw-msft/node that referenced this pull request Mar 26, 2024
PR-URL: nodejs#51929
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz.nizipli@sentry.io>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. net Issues and PRs related to the net subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants