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

AbortController abort reason Parameter #1462

Open
laurenzhonauer opened this issue Jan 18, 2022 · 7 comments · May be fixed by #1775
Open

AbortController abort reason Parameter #1462

laurenzhonauer opened this issue Jan 18, 2022 · 7 comments · May be fixed by #1775

Comments

@laurenzhonauer
Copy link

Description

In the definition of the AbortController on MDN it is stated, that it is possible to pass a parameter to AbortController.abort() that is thrown as Error if the AbortController is used in a fetch call.

Current

AbortController.abort() exists, but does not expect a parameter.

Wanted

Like done in the [@types/mongodb] implementation (see here), the optional reason parameter is also needed in the [@types/node] implementation. And i think before that makes sense, it has to be implemented here in node-fetch

Version

Im using node-fetch as a transitive dependency on version 2.6.6.
I Set up a project using next.js using ^12.0.7 and ts-node using ^10.4.0

@laurenzhonauer
Copy link
Author

Im not sure if I understood the ecosystem correctly, but i want to point out that i also opened the same issue as discussion for @types/node. I hope thats alright.

As said in the comments on the discussion: I hope i raised this correctly, if not, please help me do it right 😄

@jimmywarting
Copy link
Collaborator

jimmywarting commented Jan 23, 2022

Must say that this issue was a bit fussy, didn't exactly understood what you wanted us to do...
that's when i found AbortSignal.reason on MDN.

The signal.reason was added in node v17.2.0
so it's quite new...

@laurenzhonauer
Copy link
Author

Sorry for unclearity. I wasnt aware that its so new, because in the abort documentation they just say, that the parameter exists and I did not see that there is a specific page for AbortSignal.reason.

Yes it seems to be quite new, but it also seems quite handy to me. Thats why I opened the Issue to give the pointer. ^^

@omer727
Copy link

omer727 commented Mar 6, 2022

You can try and use the following snippet

try{
    fetch...
    } catch (error) {
if (abortController.signal.aborted) {
        return 'http request was timed out';
    }
}

@stbrody
Copy link

stbrody commented Mar 8, 2022

You can try and use the following snippet

try{
    fetch...
    } catch (error) {
if (abortController.signal.aborted) {
        return 'http request was timed out';
    }
}

this approach is incredibly imprecise if the fetch could fail for multiple reasons, or if the AbortSignal could be triggered for different reasons (e.g. timeout vs process shutdown). Unless we can actually use the reason from the AbortSignal, we risk losing useful information and error context.

So I agree with OP that it would be very valuable to have node-fetch respect the reason field in the AbortSignal and change the error message thrown accordingly.

@gomain
Copy link

gomain commented Sep 5, 2023

I believe the specifications says that, upon abort, the reason (when given) would be thrown instead of the default AbortError.

For me this is very invasive. It prevents detecting that an abort has occurred regardless of reason. A middle ground would be to set an error's cause with the reasen.

  try {
    const controller = new AbortController();
    const timeout = setTimeout(() => { controller.abort("took to long"); }, 1000 * 60);
    const response = await fetch("http://takes.very-long-time-to-resepond.com/api", {
      signal: contoller.signal,
    }).finally(() => clearTimeout(timeout));
    /* use response */
  } catch (error) {
    if (error instanceof AbortError) {
      console.log("fetch aborted");
      if (error.cause === "took to long") {
        console.log("because it took so long");
      } else {
        throw new Error("Unknown abortion", { cause: error });
      }
    } else {
      throw new Error("Unknown error", { cause: error });
    }
  }

Alternatively, the AbortError could carry a reference of the AbortSignal that triggered the abortion.

But never throw the reason.

@gomain gomain linked a pull request Sep 8, 2023 that will close this issue
2 tasks
@gomain
Copy link

gomain commented Sep 8, 2023

Node's implementation of fetch does throw the given reason, validating that it is an object otherwise a TypeError is thrown.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants