Abort signal is not respected

[2color]

When passing an new AbortController().signal to a verifiedFetch request, aborting does not seem to work.

You can see this in action in the reproduction, where if you click to fetch IPNS and right after click Abort Fetch, the controller.abort() call does not cause the promise from the verifiedFetch call to throw an error as you'd expect in Fetch

To make this easier to test, I've added a reproduction:

Reproduction

[SgtPooki]

This could be why the first request takes so long with ipfs/service-worker-gateway#122

Will investigate further tomorrow

[SgtPooki]

verified that when updated to 1.3.0, fetchIPNS and then abort result in "400" status code: https://codesandbox.io/p/sandbox/heali-verified-fetch-example-m5zmfd?file=%2Findex.html

sometimes the buttons don't seem to trigger anything, but I believe this is resolved 👍

[2color]

verifiedFetch should throw an error, i.e., not resolve the Promise, if the request is aborted to be consistent with Fetch

https://developer.mozilla.org/en-US/docs/Web/API/AbortController

Note: When abort() is called, the fetch() promise rejects with a DOMException named AbortError.

[SgtPooki]

we can't actually throw a legit AbortError, so i'll update to throw @libp2p/interface's AbortError

some context in: nodejs/node#40692

[2color]

Thanks @SgtPooki!

The only thing that's a bit burdensome about the custom AbortError is that it doesn't have the name property set to AbortError.

https://github.com/libp2p/js-libp2p/blob/d12cdce8e58012a1f01cb06c4fcc7adec1c1818f/packages/interface/src/errors.ts#L6-L19

So checking for this error either requires importing the error type:

import {AbortError} from '@libp2p/interface'
...
} catch (err) {
      if(err instanceof AbortError) {
        console.log('all good 🎉 no need to set error since the user aborted.')
}
...

I tried checking the custom code or type properties, which are defined on the AbortError type, but both of these result in a TypeScript type errors

Suggestion

Given this, I think it would be useful to follow the same convention as built-in error objects and set the name to the class name of the error. This would allow this kind of check without importing custom error types while keeping the TS compiler happy:

} catch (err) {
      if(err instanceof Error && err.name === 'AbortError') {
        console.log('all good 🎉 no need to set error since the user aborted.')
}
...

WDYT, @SgtPooki ?

[SgtPooki]

@2color that sounds good.. I think we should add a .name property to errors in libp2p.

However, I think that libp2p checks .code even though nodejs/node#40692 suggests checking the .name property

AbortError has a .code of ABORT_ERR that we can check today.

see https://codepen.io/SgtPooki/pen/bGJRoeB?editors=1011

[2color]

AbortError has a .code of ABORT_ERR that we can check today.

The problem with .code is that I haven't found a way to check it using typescript without explicitly importing the type from js-libp2p/interface or setting the e: any in the catch clause.

[achingbrain]

Normally you'd just do something like this:

} catch (err: any) {
      if (err.code === 'ABORT_ERR') {
        console.log('all good 🎉 no need to set error since the user aborted.')
}

The any is 🤮 but this is JavaScript not Java, there's nothing to stop non-Error instances being thrown (linting, yes, but it could be thrown from code you don't control) so it's a free-for-all anyway.

[SgtPooki]

AFAIK, all we should need to do here now is update to latest helia + libp2p and we should be good to go, right?

[SgtPooki]

@helia/verified-fetch is respecting abort signals now.. I think we can close this.