net: support AbortSignal in server.listen #36623

Linkgoron · 2020-12-24T23:02:30Z

net: support AbortSignal in server.listen

Add AbortSignal support to server.listen

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

benjamingr · 2020-12-24T23:05:26Z

doc/api/net.md

@@ -344,6 +347,7 @@ changes:
  * `ipv6Only` {boolean} For TCP servers, setting `ipv6Only` to `true` will
    disable dual-stack support, i.e., binding to host `::` won't make
    `0.0.0.0` be bound. **Default:** `false`.
+  * `signal` {AbortSignal} An AbortSignal that may be used to close a listening server.


a docs example would be nice - namely mentioning this calls .close internally and doesn't stop ongoing connections the server is already serving.

benjamingr

Thanks 🙏

nodejs-github-bot · 2020-12-24T23:12:57Z

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

lib/net.js

aduh95 · 2020-12-26T14:25:28Z

lib/net.js

@@ -1398,6 +1415,7 @@ Server.prototype.listen = function(...args) {
    listenInCluster(this, null, -1, -1, backlogFromArgs);
    return this;
  }
+  addAbortSignalOption(this, options);


If this function is only used once, is it relevant to have it as a separate function instead of inlining it?

I just thought that it would make the listen function harder to read.

aduh95 · 2020-12-26T14:26:57Z

lib/internal/streams/add-abort-signal.js

@@ -9,7 +9,7 @@ const eos = require('internal/streams/end-of-stream');
 const { ERR_INVALID_ARG_TYPE } = codes;

 // This method is inlined here for readable-stream
-// It also does not allow for signal to not exist on the steam
+// It also does not allow for signal to not exist on the stream


Unrelated change, but good catch ;) it might be worth having it land in a separate PR (in case this one was reverted or didn't get backported to LTS release lines).

lib/net.js

nodejs-github-bot · 2020-12-27T18:13:03Z

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

github-actions · 2020-12-31T16:08:10Z

Commit Queue failed

- Loading data for nodejs/node/pull/36623
✔  Done loading data for nodejs/node/pull/36623
----------------------------------- PR info ------------------------------------
Title      net: support AbortSignal in server.listen (#36623)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     Linkgoron:net-server-listen-abortcontroller -> nodejs:master
Labels     author ready, net, semver-minor
Commits    1
 - net: support abortSignal in server.listen
Committers 1
 - Nitzan Uziely 
PR-URL: https://github.com/nodejs/node/pull/36623
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36623
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-27T18:13:03Z: https://ci.nodejs.org/job/node-test-pull-request/35096/
- Querying data for job/node-test-pull-request/35096/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Thu, 24 Dec 2020 23:02:30 GMT
   ✔  Approvals: 1
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/36623#pullrequestreview-560508048
   ✖  This PR needs to wait 6 more hours to land (or 0 hours if there is one more approval)
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu

https://github.com/nodejs/node/actions/runs/454778704

jasnell · 2020-12-31T20:44:50Z

Landed in 51b4367

PR-URL: #36623 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) [#29412](#29412) * support AbortSignal in fork (Benjamin Gruenbaum) [#36603](#36603) * crypto: * implement basic secure heap support (James M Snell) [#36779](#36779) * fixup bug in keygen error handling (James M Snell) [#36779](#36779) * introduce X509Certificate API (James M Snell) [#36804](#36804) * implement randomuuid (James M Snell) [#36729](#36729) * doc: * update release key for Danielle Adams (Danielle Adams) [#36793](#36793) * add dnlup to collaborators (Daniele Belardi) [#36849](#36849) * add panva to collaborators (Filip Skokan) [#36802](#36802) * add yashLadha to collaborator (Yash Ladha) [#36666](#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) [#36685](#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) [#36623](#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) [#34291](#34291) * v8: * fix native constructors (ExE Boss) [#36549](#36549)

PR-URL: #36889 Notable changes: * child_process: * add 'overlapped' stdio flag (Thiago Padilha) (#29412) * support AbortSignal in fork (Benjamin Gruenbaum) (#36603) * crypto: * implement basic secure heap support (James M Snell) (#36779) * fixup bug in keygen error handling (James M Snell) (#36779) * introduce X509Certificate API (James M Snell) (#36804) * implement randomuuid (James M Snell) (#36729) * doc: * update release key for Danielle Adams (Danielle Adams) (#36793) * add dnlup to collaborators (Daniele Belardi) (#36849) * add panva to collaborators (Filip Skokan) (#36802) * add yashLadha to collaborator (Yash Ladha) (#36666) * http: * set lifo as the default scheduling strategy in Agent (Matteo Collina) (#36685) * net: * support abortSignal in server.listen (Nitzan Uziely) (#36623) * process: * add direct access to rss without iterating pages (Adrien Maret) (#34291) * v8: * fix native constructors (ExE Boss) (#36549)

nodejs-github-bot added the net Issues and PRs related to the net subsystem. label Dec 24, 2020

Linkgoron changed the title ~~net: support abortSignal in server.listen~~ net: support AbortSignal in server.listen Dec 24, 2020

benjamingr reviewed Dec 24, 2020

View reviewed changes

benjamingr added request-ci Add this label to start a Jenkins CI on a PR. semver-minor PRs that contain new features and should be released in the next minor version. labels Dec 24, 2020

benjamingr approved these changes Dec 24, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 24, 2020

aduh95 reviewed Dec 26, 2020

View reviewed changes

benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

net: support abortSignal in server.listen

8bfc031

Linkgoron force-pushed the net-server-listen-abortcontroller branch from f7e8509 to 8bfc031 Compare December 27, 2020 17:48

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020

benjamingr added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 27, 2020

benjamingr approved these changes Dec 31, 2020

View reviewed changes

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 31, 2020

benjamingr removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 31, 2020

jasnell approved these changes Dec 31, 2020

View reviewed changes

jasnell closed this Dec 31, 2020

jasnell pushed a commit that referenced this pull request Dec 31, 2020

net: support abortSignal in server.listen

51b4367

PR-URL: #36623 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams pushed a commit that referenced this pull request Jan 12, 2021

net: support abortSignal in server.listen

02dbcc4

PR-URL: #36623 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

danielleadams mentioned this pull request Jan 12, 2021

v15.6.0 proposal #36889

Merged

Linkgoron deleted the net-server-listen-abortcontroller branch January 22, 2021 22:08

targos added dont-land-on-v12.x labels Aug 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

net: support AbortSignal in server.listen #36623

net: support AbortSignal in server.listen #36623

Linkgoron commented Dec 24, 2020 •

edited

Loading

benjamingr Dec 24, 2020

Linkgoron Dec 27, 2020

benjamingr left a comment

nodejs-github-bot commented Dec 24, 2020

aduh95 Dec 26, 2020

Linkgoron Dec 27, 2020

aduh95 Dec 26, 2020

nodejs-github-bot commented Dec 27, 2020

github-actions bot commented Dec 31, 2020

jasnell commented Dec 31, 2020 •

edited

Loading

net: support AbortSignal in server.listen #36623

net: support AbortSignal in server.listen #36623

Conversation

Linkgoron commented Dec 24, 2020 • edited Loading

benjamingr Dec 24, 2020

Choose a reason for hiding this comment

Linkgoron Dec 27, 2020

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 24, 2020

aduh95 Dec 26, 2020

Choose a reason for hiding this comment

Linkgoron Dec 27, 2020

Choose a reason for hiding this comment

aduh95 Dec 26, 2020

Choose a reason for hiding this comment

nodejs-github-bot commented Dec 27, 2020

github-actions bot commented Dec 31, 2020

jasnell commented Dec 31, 2020 • edited Loading

Linkgoron commented Dec 24, 2020 •

edited

Loading

jasnell commented Dec 31, 2020 •

edited

Loading