[WIP] Add abort support #437
Conversation
Codecov Report
@@ Coverage Diff @@
## master #437 +/- ##
==========================================
- Coverage 100% 95.22% -4.78%
==========================================
Files 6 6
Lines 503 544 +41
Branches 153 164 +11
==========================================
+ Hits 503 518 +15
- Misses 0 21 +21
- Partials 0 5 +5
Continue to review full report at Codecov.
|
| if (signal.aborted) { | ||
| abortController.abort(); | ||
| } else { | ||
| signal.addEventListener('abort', () => { |
There was a problem hiding this comment.
This eventListener is never removed, is it?
There was a problem hiding this comment.
If it could be of any help maybe adding {once: true} to addEventListener could help cleaning up things
There was a problem hiding this comment.
@mika-fischer No, and it's very difficult to do so. We could add a { once: true } though.
There was a problem hiding this comment.
But won't this cause issues, for instance in the case where I have a long running Node.js application where I have a global AbortController (to be able to abort all requests when the application shuts down) and each request in the whole lifetime of the app (let's say hundreds of thousands) adds a listener, none of which is ever removed?
There was a problem hiding this comment.
😔 yes. It’s also one of the reasons why this is a WIP.
There was a problem hiding this comment.
Can't you just save a cleanup closure (which just removes the listener from the parent AbortController) and attach this closure to the appropriate events of the Node.js Request/Response so that it gets called when the request is finished?
There was a problem hiding this comment.
A cleanup closure would remove the listener from the parent AbortController after fetch() terminates for any reason, right? Would that affect the ability to cancel a fetch that reuses a Request object for multiple fetches? Does that maybe mean that the listener should be added each time the fetch begins and removed when the fetch terminates?
There was a problem hiding this comment.
.once() seems like it would only help if the request terminated by being aborted, but I might be missing something.
|
Hi there. I would love to see this happen. Is there any way I can assist? |
|
Hi @RikkiGibson, yes! The biggest thing right now is making sure the "abort" signal handlers get cleaned up correctly, i.e., #437 (comment). But I'd appreciate any help in adding tests as well. You could submit a pull request against the |
| @@ -82,11 +100,23 @@ export default class Request { | |||
| } | |||
| } | |||
|
|
|||
| const abortController = new AbortController(); | |||
There was a problem hiding this comment.
Could someone explain why we are initializing another internal AbortController?
There was a problem hiding this comment.
Per spec, the AbortSignal we expose through signal must be a sanitized object, and not be the signal passed in. To create a new AbortSignal we have to create another AbortController.
|
I haven't been able to find the spec that require signal to be exposed. Where can I find this information? |
|
https://fetch.spec.whatwg.org/#request-class Look for the Request constructor. Something like step 30 it will mention that the passed in signal must be used to create a “following” signal, i.e. one that subscribes to a parent signal (given by the user) but can also be canceled independent of the parent signal. |
| @@ -39,17 +39,35 @@ export default function fetch(url, opts) { | |||
| return new fetch.Promise((resolve, reject) => { | |||
| // build request object | |||
| const request = new Request(url, opts); | |||
| const signal = getAbortSignal(request); | |||
| if (signal.aborted) { | |||
| reject(new FetchError(`Fetch to ${request.url} has been aborted`, 'aborted')); | |||
There was a problem hiding this comment.
any way this could be more inline with the browser spec? rejecting with an AbortError? when using with isomorphic fetch this would result in having to catch two types of errors for node and browser
|
A shoutout to people watching this thread, we are closed to merging AbortSignal support, it's a bit different from previous WIP, so let us know if you have any feedback on it: #539 |
This is still a WIP:
AbortSignaltype check after Make AbortSignal and AbortController stringify correctly mysticatea/abort-controller#5 is applied.Note: this PR will introduce a runtime dependency on https://github.com/mysticatea/abort-controller.
Fixes: #95