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

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

Closed

Conversation

benjamingr
Copy link
Member

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

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Nov 9, 2020
@benjamingr benjamingr requested a review from ronag November 9, 2020 10:53
@benjamingr benjamingr changed the title add support for abortsignal to http.request http: add support for abortsignal to http.request Nov 9, 2020
Comment on lines 77 to 78
const server = http.createServer(common.mustNotCall((req, res) => {
}));
Copy link
Member

Choose a reason for hiding this comment

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

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

doc/api/http.md Outdated
@@ -2596,6 +2601,10 @@ events will be emitted in the following order:
Setting the `timeout` option or using the `setTimeout()` function will
not abort the request or do anything besides add a `'timeout'` event.

Passing an `AbortSignal` and then calling `abort` on the corrosponding
Copy link
Member

Choose a reason for hiding this comment

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

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

if (options.signal) {
validateAbortSignal(options.signal, 'options.signal');
const listener = (e) => this.destroy();
options.signal.addEventListener('abort', listener);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

doc/api/http.md Outdated
@@ -2336,6 +2336,9 @@ This can be overridden for servers and client requests by passing the
<!-- YAML
added: v0.3.6
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/319999
Copy link
Contributor

Choose a reason for hiding this comment

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

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



{
// destroy with AbortSignal
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// destroy with AbortSignal
// Destroy with AbortSignal

doc/api/http.md Outdated
@@ -2596,6 +2601,10 @@ events will be emitted in the following order:
Setting the `timeout` option or using the `setTimeout()` function will
not abort the request or do anything besides add a `'timeout'` event.

Passing an `AbortSignal` and then calling `abort` on the corresponding
`AbortConttoller` will behave the same way as calling `.destroy()` on the
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -169,6 +172,14 @@ function ClientRequest(input, options, cb) {
if (options.timeout !== undefined)
this.timeout = getTimerDuration(options.timeout, 'timeout');

if (options.signal) {
Copy link
Member

Choose a reason for hiding this comment

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

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

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
const signal = options.signal;
if (signal) {
validateAbortSignal(signal, 'options.signal');
const listener = (e) => this.destroy();
Copy link
Member

Choose a reason for hiding this comment

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

I would call .destroy() with an error.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

cc @ronag

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ronag ronag left a comment

Choose a reason for hiding this comment

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

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().

@benjamingr
Copy link
Member Author

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.

@ronag
Copy link
Member

ronag commented Nov 9, 2020

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.

@benjamingr
Copy link
Member Author

benjamingr commented Nov 9, 2020

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

@benjamingr benjamingr added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 9, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Nov 9, 2020

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

@benjamingr
Copy link
Member Author

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

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member

ronag commented Nov 11, 2020

@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.

@benjamingr
Copy link
Member Author

@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)

@benjamingr benjamingr removed the blocked PRs that are blocked by other issues or PRs. label Nov 19, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@benjamingr benjamingr added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 19, 2020
@github-actions
Copy link
Contributor

Landed in a46b21f...2097ffd

@github-actions github-actions bot closed this Nov 19, 2020
nodejs-github-bot pushed a commit that referenced this pull request Nov 19, 2020
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>
MadaraUchiha added a commit to MadaraUchiha/node that referenced this pull request Nov 21, 2020
- Builds on nodejs#36048 and nodejs#36084
- Modify test to verify this fact
codebytere pushed a commit that referenced this pull request Nov 22, 2020
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>
codebytere added a commit that referenced this pull request Nov 22, 2020
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
@codebytere codebytere mentioned this pull request Nov 22, 2020
codebytere added a commit that referenced this pull request Nov 24, 2020
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
codebytere added a commit that referenced this pull request Nov 24, 2020
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
codebytere added a commit that referenced this pull request Nov 24, 2020
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
targos pushed a commit to targos/node that referenced this pull request Apr 24, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Apr 26, 2021
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>
targos pushed a commit to targos/node that referenced this pull request Apr 30, 2021
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>
targos pushed a commit that referenced this pull request Apr 30, 2021
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>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants