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

AbortSignal.timeout inconsistently leads to TimeoutError or AbortError #2171

Closed
cancan101 opened this issue Jun 26, 2023 · 14 comments · Fixed by #2172
Closed

AbortSignal.timeout inconsistently leads to TimeoutError or AbortError #2171

cancan101 opened this issue Jun 26, 2023 · 14 comments · Fixed by #2172
Labels
bug Something isn't working

Comments

@cancan101
Copy link

Bug Description

If fetch is passed a AbortSignal.timeout(timeout) as the signal and the timeout occurs, a TimeoutError is raised in certain cases whereas in other cases a AbortError is raised. The TimeoutError occurs when the fetch itself times out whereas the AbortError is raised if the timeout occurs while streaming the response.

Reproducible By

export async function fetchWithTimeout(
  resource: RequestInfo,
  options: RequestInit & { timeout?: number } = {}
) {
  const { timeout = 8000 } = options;

  const response = await fetch(resource, {
    ...options,
    signal: AbortSignal.timeout(timeout),
  });
  return response;
}

async function main() {
  const response = await fetchWithTimeout("https://google.com/", {
    timeout: 1000,
  });

  if (!response.body) {
    return null;
  }

  const saver = new WritableStream<string>({
    async write(data, controller) {
      await new Promise((r) => setTimeout(r, 1000));
    },
  });

  return await response.body.pipeThrough(new TextDecoderStream()).pipeTo(saver);
}

main().catch((x) => console.error(x));

Expected Behavior

An error of the format:

DOMException [TimeoutError]: The operation was aborted due to timeout
    at Object.fetch (node:internal/deps/undici/undici:11457:11)
...

Logs & Screenshots

Instead we get:

DOMException [AbortError]: The operation was aborted.
    at new DOMException (node:internal/per_context/domexception:53:5)
    at Fetch.abort (node:internal/deps/undici/undici:10586:19)
    at EventTarget.requestObject.signal.addEventListener.once (node:internal/deps/undici/undici:10620:22)
    at EventTarget.[nodejs.internal.kHybridDispatch] (node:internal/event_target:761:20)
    at EventTarget.dispatchEvent (node:internal/event_target:703:26)
    at abortSignal (node:internal/abort_controller:314:10)
    at AbortController.abort (node:internal/abort_controller:332:5)
    at EventTarget.abort (node:internal/deps/undici/undici:7174:18)
    at EventTarget.[nodejs.internal.kHybridDispatch] (node:internal/event_target:761:20)
    at EventTarget.dispatchEvent (node:internal/event_target:703:26)

Environment

MacOS: 12.6
Node: v19.9.0

Additional context

@cancan101 cancan101 added the bug Something isn't working label Jun 26, 2023
@ronag
Copy link
Member

ronag commented Jun 26, 2023

I'm not sure I see a problem here? The current behavior makes sense to me.

@cancan101
Copy link
Author

cancan101 commented Jun 26, 2023

I would expect to get a TimeoutError in both cases, ie whether the delay occurs in the actual fetching vs the processing of the stream since the abort is coming from the same AbortSignal.timeout.

You can see the alternative error by shortening the timeout on the AbortSignal:

  const response = await fetchWithTimeout("https://google.com/", {
    timeout: 10,
  });

which will then cause a TimeoutError rather than the AbortError.

@ronag
Copy link
Member

ronag commented Jun 26, 2023

I'm pretty sure this is spec compliant. AbortSignal.timeout is supposed to cause an abort error. Do you see a different behavior in e.g. Chrome?

@ronag
Copy link
Member

ronag commented Jun 26, 2023

@KhafraDev
Copy link
Member

fetch is expected to only throw an AbortError (previous discussion about this starts here).

@cancan101
Copy link
Author

That is definitely not the case right now. As mentioned above fetch will throw TimeoutError in certain circumstances. I think this was a new feature as of node 19. Prior versions did throw just AbortError. That being said, I don't follow why you would not want to have a differentiated TimeoutError for timeouts, consistent with the linked spec above.

@cancan101
Copy link
Author

Also the linked comment refers to user supplied reasons which isn't the issue that I have here.

@cancan101
Copy link
Author

cancan101 commented Jun 26, 2023

this seems to indicate the functionality was added to the spec: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/timeout_static

and running this on FIrefox (on Google):

fetch("https://www.google.com/", {signal: AbortSignal.timeout(100)}).catch(e => console.log(e.name)) 

yields TimeoutError

@KhafraDev
Copy link
Member

I'm inclined to think that is a bug in Firefox, the spec doesn't mention TimeoutError at all. It mentions AbortError for timeouts.

@cancan101
Copy link
Author

The DOM spec does mention it, however:
image

@cancan101
Copy link
Author

and the fetch spec seems to indicate that AbortError is used as the fallbackError:
image

@ronag
Copy link
Member

ronag commented Jun 26, 2023

I'm inclined to think that is a bug in Firefox, the spec doesn't mention TimeoutError at all. It mentions AbortError for timeouts.

I'm leaning towards that you might be wrong here...

@KhafraDev
Copy link
Member

Yeah I was, we weren't forwarding the reason. Will have a PR up soon.

KhafraDev added a commit to KhafraDev/undici that referenced this issue Jun 26, 2023
KhafraDev added a commit that referenced this issue Jun 27, 2023
* fix: forward error reason to fetch controller

Fixes #2171

* fix: update spec text

* fixup
@Dzieni
Copy link

Dzieni commented Aug 2, 2023

Great to see that it's fixed. Can't wait to have it oficially released. Is there any release schedule?

crysmags pushed a commit to crysmags/undici that referenced this issue Feb 27, 2024
* fix: forward error reason to fetch controller

Fixes nodejs#2171

* fix: update spec text

* fixup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants