http: add support for abortsignal to http.request by benjamingr · Pull Request #36048 · nodejs/node · GitHub

cc @mcollina @jasnell @addaleax

This is the last one from #35877 (comment) - if you have any more APIs you want me to add support for AbortSignal in before I go back to that list I started making back then please let me know :]

Also we should talk about streams and this probably.

Checklist

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

Review requested:

@nodejs/http
@nodejs/net

Suggested change

      
              const server = http.createServer(common.mustNotCall((req, res) => {
          
              }));
          
              const server = http.createServer(common.mustNotCall());

Suggested change

      
            Passing an `AbortSignal` and then calling `abort` on the corrosponding
          
            Passing an `AbortSignal` and then calling `abort` on the corresponding

Nit: perhaps use once here as well ({ once: true })?

I feel like it would be more hacky since we're removing the listener below even if it is invoked since close should be fired.

If I pass once and someone abortss the controller - the signal will fire abort, the listener will be removed, then close will fire and we will try removing a non-existing listener which will technically be no harm but will be slightly confusing.

Suggested change

      
                pr-url: https://github.com/nodejs/node/pull/319999
          
                pr-url: https://github.com/nodejs/node/pull/36048

Suggested change

      
              // destroy with AbortSignal
          
              // Destroy with AbortSignal

Suggested change

      
            `AbortConttoller` will behave the same way as calling `.destroy()` on the
          
            `AbortController` will behave the same way as calling `.destroy()` on the

Should we extract the value in a variable to be safe? Options could be changed later or the property could be a getter that throws afterwards

I would call .destroy() with an error.

I... think you're right. If cancellation is abnormal elsewhere it should likely be abnormal termination here and abort with an AbortError with a .code

I... think I changed my mind again. Lol, I think calling .destroy without an error is probably right and the error the user is getting should probably be ECONNRESET and not an AbortError anyway.

I'd rather users get consistent errors on request aborts than always getting AbortErrors consistent errors when using AbortController.

I'm really not sure here tbh.

cc @ronag

I don't think it will ECONNRESET if aborted before a socket is assigned.

@ronag it does (there is a test for it)

cc @jasnell

This is a bit weird to me... if we are going to do this isn't it better to just add signal support generically to streams?

I don't really see the point of this since streams have .destroy().

if we are going to do this isn't it better to just add signal support generically to streams?

@ronag even if http.request returns a stream and we make those cancelable with signals, it should still take a signal as a parameter for ownership.

If we make stream.Readable cancellable with signals through a AbortSignal known symbol property or constructor parameter that would be useful but not help in this case since the stream isn't constructed by the user - it's just returned to the caller.

Allowing users to destroy an http.request (and other streams) by passing in a signal is very useful ergonomics wise. It's the same way users cancel other things (like timers) and the standard web platform cancellation mechanism.

Allowing users to destroy an http.request (and other streams) by passing in a signal is very useful ergonomics wise. It's the same way users cancel other things (like timers) and the standard web platform cancellation mechanism.

I don't quite see if but don't have a strong opinion. However, I do think if we do this we should do it on the streams level.

I will follow up with a PR for readable

Edit: very much wip @ronag https://github.com/nodejs/node/compare/master...benjamingr:abort-signal-stream?expand=1 will make a PR sometime this week hopefully

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

Why not have this PR also destroy with lazyDOMException('The operation was aborted', 'AbortError')?

@ronag I think users would probably expect an ECONNRESET - no?

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

@ronag I think users would probably expect an ECONNRESET - no?

Not necessarily. What about the case when aborting before 'socket'? I would not expect a ECONNRESET then.

I don't have a strong opinion.

@ronag the test here aborts synchronously, that is before socket iiuc.

Note I am not sure what's the right thing either - and also in http2, I don't understand the implications of why aborting with an error here would matter so if there is a good reason to .destroy with an error here (and in HTTP2) please let me know :]

My involvement in the project and willingness to do the work does not excuse my ignorance in this case. So if I'm missing something please let me know.

(The more general AbortSignal for Readable will .destroy with an error)

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

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

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

Landed in a46b21f...2097ffd

PR-URL: #36048 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

- Builds on nodejs#36048 and nodejs#36084 - Modify test to verify this fact

PR-URL: #36048 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

Notable changes: dns: * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099 events: * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001 http: * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048 http2: * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 lib: * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716 path: * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962 readline: * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675 src: * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940 util: * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055 PR-URL: TODO

Notable changes: dns: * (SEMVER-MINOR) add a cancel() method to the promise Resolver (Szymon Marczak) #33099 events: * (SEMVER-MINOR) add max listener warning for EventTarget (James M Snell) #36001 http: * (SEMVER-MINOR) add support for abortsignal to http.request (Benjamin Gruenbaum) #36048 http2: * (SEMVER-MINOR) allow setting the local window size of a session (Yongsheng Zhang) #35978 lib: * (SEMVER-MINOR) add throws option to fs.f/l/statSync (Andrew Casey) #33716 path: * (SEMVER-MINOR) add `path/posix` and `path/win32` alias modules (ExE Boss) #34962 readline: * (SEMVER-MINOR) add getPrompt to get the current prompt (Mattias Runge-Broberg) #33675 src: * (SEMVER-MINOR) add loop idle time in diagnostic report (Gireesh Punathil) #35940 util: * (SEMVER-MINOR) add `util/types` alias module (ExE Boss) #34055 PR-URL: #36232

PR-URL: nodejs#36048 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

PR-URL: #36048 Backport-PR-URL: #38386 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Denys Otrishko <shishugi@gmail.com> Reviewed-By: Ricky Zhou <0x19951125@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 9, 2020

benjamingr requested a review from ronag November 9, 2020 10:53

benjamingr changed the title add support for abortsignal to http.request http: add support for abortsignal to http.request Nov 9, 2020

benjamingr force-pushed the add-abortsignal-http-request branch from aa6a902 to bb957b5 Compare November 9, 2020 10:54

addaleax approved these changes Nov 9, 2020

View reviewed changes

lundibundi approved these changes Nov 9, 2020

View reviewed changes

rickyes approved these changes Nov 9, 2020

View reviewed changes

benjamingr force-pushed the add-abortsignal-http-request branch from bb957b5 to 728d4ad Compare November 9, 2020 11:28

targos reviewed Nov 9, 2020

View reviewed changes

benjamingr force-pushed the add-abortsignal-http-request branch from 728d4ad to 89c38be Compare November 9, 2020 11:39

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020

Trott approved these changes Nov 9, 2020

View reviewed changes

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020

mcollina reviewed Nov 9, 2020

View reviewed changes

ronag reviewed Nov 9, 2020

View reviewed changes

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

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020

MadaraUchiha mentioned this pull request Nov 10, 2020

http2: add support for AbortSignal to http2Session.request #36070

Closed

4 tasks

benjamingr removed the blocked PRs that are blocked by other issues or PRs. label Nov 19, 2020

benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2020

github-actions Bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2020

github-actions Bot closed this Nov 19, 2020

MadaraUchiha added a commit to MadaraUchiha/node that referenced this pull request Nov 21, 2020

http2: Abort calls destroy with an AbortError

dc99724

- Builds on nodejs#36048 and nodejs#36084 - Modify test to verify this fact

codebytere mentioned this pull request Nov 22, 2020

v15.3.0 proposal #36232

Merged

targos added the backport-open-v14.x label Apr 24, 2021

targos mentioned this pull request Apr 24, 2021

[v14.x] Backport AbortController and friends #38386

Closed

targos added backported-to-v14.x and removed backport-open-v14.x labels Apr 30, 2021

danielleadams mentioned this pull request May 3, 2021

v14.17.0 proposal #38507

Merged

aicanfly mentioned this pull request May 25, 2021

Error: socket hang up with 15.7.0 or newer #37510

Closed

coderabbitai Bot mentioned this pull request Jul 29, 2026

release: v1.11.0 — weight tolerance 10%, sync health alert repaired malisanjay475/jms_enterprise#1087

Merged

	const server = http.createServer(common.mustNotCall((req, res) => {
	}));
	const server = http.createServer(common.mustNotCall());

	Passing an `AbortSignal` and then calling `abort` on the corrosponding
	Passing an `AbortSignal` and then calling `abort` on the corresponding

	pr-url: https://github.com/nodejs/node/pull/319999
	pr-url: https://github.com/nodejs/node/pull/36048

	`AbortConttoller` will behave the same way as calling `.destroy()` on the
	`AbortController` will behave the same way as calling `.destroy()` on the

http: add support for abortsignal to http.request by benjamingr · Pull Request #36048 · nodejs/node · GitHub

Uh oh!

Uh oh!

http: add support for abortsignal to http.request - #36048

http: add support for abortsignal to http.request#36048benjamingr wants to merge 1 commit intonodejs:masternodejs/node:masterfrom benjamingr:add-abortsignal-http-requestbenjamingr/io.js:add-abortsignal-http-requestCopy head branch name to clipboard

Conversation

benjamingr commented Nov 9, 2020

Checklist

Uh oh!

nodejs-github-bot commented Nov 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ronag left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Nov 9, 2020

Uh oh!

ronag commented Nov 9, 2020

Uh oh!

benjamingr commented Nov 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 9, 2020

Uh oh!

ronag commented Nov 9, 2020

Uh oh!

benjamingr commented Nov 9, 2020

Uh oh!

nodejs-github-bot commented Nov 10, 2020

Uh oh!

ronag commented Nov 11, 2020

Uh oh!

benjamingr commented Nov 11, 2020

Uh oh!

nodejs-github-bot commented Nov 19, 2020

Uh oh!

nodejs-github-bot commented Nov 19, 2020

Uh oh!

nodejs-github-bot commented Nov 19, 2020

Uh oh!

github-actions Bot commented Nov 19, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

http: add support for abortsignal to http.request#36048
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:add-abortsignal-http-request

ronag left a comment •
edited
Loading

benjamingr commented Nov 9, 2020 •
edited
Loading