Add optional abort signal to fetch json and merge with timeout #2068
Conversation
stephhuynh18
requested review from
stbrody and
ukstv
and removed request for
stbrody
There was a problem hiding this comment.
mostly looks good, but I'd like to find a way to take advantage of the reason arg to AbortController.abort so that we can have more meaningful error messages, but I'm having trouble getting it to work in my own testing. I left some questions for @ukstv to see if he has any ideas
| return new Promise((resolve) => { | ||
| id = setTimeout(() => { | ||
| resolve(doc) | ||
| }, 2000) |
There was a problem hiding this comment.
maybe bump this up to 5 seconds so there's no chance of a race with the timedAbortSignal
| }) | ||
| }) | ||
|
|
||
| const timedAbortSignal = createTimedAbortSignal(5000) |
There was a problem hiding this comment.
maybe don't use a timed signal but a regular signal here to make it clear that the idea is that it never gets triggered and the timeout runs instead?
| @@ -384,11 +391,78 @@ describe('Ceramic interop: core <> http-client', () => { | |||
| fetchJson(`http://localhost:${daemon.port}/api/v0/streams/${doc.id}/content`, { | |||
| timeout: 1000, | |||
| }) | |||
| ).rejects.toThrow(`Http request timed out after 1000 ms`) | |||
There was a problem hiding this comment.
I'm frustrated by this loss of context. Looking at the node docs, it seems like AbortController.abort should be able to take an option string arg reason which could be used to provide a context string that the AbortSignal will include when it's aborted, and which can be used to give more meaningful error messages when an operation is cancelled. Sounds great! But in my testing I'm not actually able to get it to work - I keep getting an error Expected 0 arguments, but got 1.
There was a problem hiding this comment.
Update: I played around with different versions node using nvm. Node 16.14.0 does indeed have the reason arg, but Node 16.13.1 doesn't seem to have it. So I guess this is a very recent addition.
But even though I can select node 16.14.0 with NVM and see it working by running node manually, when I try to use it in Ceramic I still get a compilation error Expected 0 arguments, but got 1. Any ideas why that would be @ukstv? Does that mean my Ceramic build is still using Node 16.13.1 for some reason, even though node --version is showing 16.14.0?
There was a problem hiding this comment.
@stbrody 1. Expected 0 arguments - if it is in TS, then Node.js types do not include reason parameter in the declarations. On my end vanilla Node.js (both 16.13 and 16.14) in REPL allow me to specify the reason.
2. I tend to believe, it does not matter if we pass a reason there to the AbortController. node-fetch does not respect reason anyway.
There was a problem hiding this comment.
We could do something like node-fetch/node-fetch#1462 (comment) though @stephhuynh18
There was a problem hiding this comment.
And, yes, it is a recent addition: https://nodejs.org/en/blog/release/v16.14.0/
There was a problem hiding this comment.
I used to do something like node-fetch/node-fetch#1462 (comment) but with this change we won't be able to tell if the request aborted because of a timeout or because the user called abort themselves.
There was a problem hiding this comment.
I used to do something like node-fetch/node-fetch#1462 (comment) but with this change we won't be able to tell if the request aborted because of a timeout or because the user called abort themselves.
Not good.
Current code in the PR is indeed the most appropriate.
There was a problem hiding this comment.
looks like there's a bunch of open issues on various repos to get this more fully supported. Agreed it's not worth doing anything more on this now, but we should revisit in the future once there's better support for abort reasons throughout the node ecosystem
| clear: () => void | ||
| } | ||
|
|
||
| export function createTimedAbortSignal(timeout: number): TimedAbortSignal { |
There was a problem hiding this comment.
Weak suggestion: use class instead of function and interface. I do not know why, just have a feeling, it might be easier to support.
| @@ -384,11 +391,78 @@ describe('Ceramic interop: core <> http-client', () => { | |||
| fetchJson(`http://localhost:${daemon.port}/api/v0/streams/${doc.id}/content`, { | |||
| timeout: 1000, | |||
| }) | |||
| ).rejects.toThrow(`Http request timed out after 1000 ms`) | |||
There was a problem hiding this comment.
looks like there's a bunch of open issues on various repos to get this more fully supported. Agreed it's not worth doing anything more on this now, but we should revisit in the future once there's better support for abort reasons throughout the node ecosystem
stephhuynh18
deleted the
feature/net-1229-add-optional-abortsignal-to-fetchjson
branch
Description
Created util to merge abort signals (did not use https://www.npmjs.com/package/composite-abort-controller as it waits for ALL abort signals in the composite-abort-signal to abort instead of "one of")
Created utility to make timed abort signals
Added optional abort signal to fetch json (following signal naming convention in ipfs)
How Has This Been Tested?
Added tests for fetch json in
CLIpackage ascommonpackage does not have dedicated testing