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

Request object's AbortSignal does not abort with the expected reason #43874

Closed
cambecc opened this issue Jul 17, 2022 · 5 comments · Fixed by nodejs/undici#1580
Closed

Request object's AbortSignal does not abort with the expected reason #43874

cambecc opened this issue Jul 17, 2022 · 5 comments · Fixed by nodejs/undici#1580
Labels
abortcontroller Issues and PRs related to the AbortController API fetch Issues and PRs related to the Fetch API

Comments

@cambecc
Copy link

cambecc commented Jul 17, 2022

Version

v18.5.0

Platform

Linux foo 5.15.0-41-generic #44-Ubuntu SMP Wed Jun 22 14:20:53 UTC 2022 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

const signal1 = AbortSignal.timeout(100);
const request = new Request("https://example.com", {signal: signal1});
const signal2 = request.signal;

console.log("same?", signal1 === signal2);  // false

signal1.addEventListener("abort", e => console.log("1", e.target.reason));  // TimeoutError
signal2.addEventListener("abort", e => console.log("2", e.target.reason));  // AbortError

setTimeout(() => {}, 200);  // wait for the abort to happen

How often does it reproduce? Is there a required condition?

Always.

What is the expected behavior?

The Request's signal should abort with the same reason as the signal provided during Request construction. (In this case, a TimeoutError.)

What do you see instead?

The Request's signal aborts with a different reason from the signal provided during construction. In this repro, the TimeoutError is converted to AbortError, which is a loss of information.

same? false
2 DOMException [AbortError]: This operation was aborted
    at new DOMException (node:internal/per_context/domexception:53:5)
    at AbortController.abort (node:internal/abort_controller:320:18)
    at AbortSignal.abort (node:internal/deps/undici/undici:4974:36)
    at [nodejs.internal.kHybridDispatch] (node:internal/event_target:645:20)
    at AbortSignal.dispatchEvent (node:internal/event_target:587:26)
    at abortSignal (node:internal/abort_controller:292:10)
    at Timeout._onTimeout (node:internal/abort_controller:112:7)
    at listOnTimeout (node:internal/timers:564:17)
    at process.processTimers (node:internal/timers:507:7)
1 DOMException [TimeoutError]: The operation was aborted due to timeout
    at new DOMException (node:internal/per_context/domexception:53:5)
    at Timeout._onTimeout (node:internal/abort_controller:114:9)
    at listOnTimeout (node:internal/timers:564:17)
    at process.processTimers (node:internal/timers:507:7)

FWIW, recent versions of both Chrome and Firefox perform as expected (TimeoutError seen by both event handlers).

Additional information

It seems the signal provided during Request construction is wrapped by a new abort controller/signal (see the output: "same? false"). Is the reason of the underlying signal passed to the new controller's abort() call? Looks like no.

@LiviaMedeiros
Copy link
Contributor

I believe this is spec issue: abort fetch part demands AbortError DOMException and nothing else.

There was an ongoing work on throwing signal.reason if it's provided and generic AbortError if not: whatwg/fetch#1343

Although I'd absolutely prefer throwing AbortError and have signal.reason as its cause property instead.

@LiviaMedeiros LiviaMedeiros added abortcontroller Issues and PRs related to the AbortController API fetch Issues and PRs related to the Fetch API labels Jul 17, 2022
@cambecc
Copy link
Author

cambecc commented Jul 17, 2022

I think this is a bug with Request construction not satisfying the spec because new Request(input, init) should have this behavior:

  1. If signal is not null, then make this’s signal follow signal.

The definition of follow requires that the Request's signal aborts with the same reason as the parent signal. So, from the repro code above, signal2 should follow signal1 and abort with TimeoutError.

@cambecc
Copy link
Author

cambecc commented Jul 17, 2022

Although this issue doesn't involve the fetch function, I understand the point about abort fetch demanding AbortError. I haven't been able to find any issues opened against the fetch spec to accommodate AbortSignal.timeout() and TimeoutError, even though that seems to be the primary motivator: whatwg/dom#1032

@LiviaMedeiros
Copy link
Contributor

new Request(input, init) should have this behavior

Yep, it seems so.
This step should be handled by https://github.com/nodejs/undici/blob/b6af4e6eb5177444bc91f740b68de4eb8a43c561/lib/fetch/request.js#L369-L375. ac.abort() is called without the reason passed to it. Would you like to send a PR to undici to address this problem?

I haven't been able to find any issues opened against the fetch spec to accommodate AbortSignal.timeout() and TimeoutError,

Right now it looks like spec is softly contradicting itself there. Implementations of TimeoutError in browsers are pretty fresh and they follow the current version; e.g. you can try this in Chrome and Firefox and see timeout being replaced with generic AbortError:

fetch('https://github.com/nodejs/node/issues/43874', {
  signal: AbortSignal.timeout(1)
}).catch(err => {
  console.error(err.name, err.code, err.message, err.cause);
});

From my understanding, this is issue you're searching for: whatwg/dom#1030. fetch spec is the only stalled part of it.

Also, anticipated compatibility breakage is partially addressed in whatwg/dom#1090: since TimeoutError already has implementations released and they throw ABORT_ERR/AbortError instead of TIMEOUT_ERR/TimeoutError, adjustments to fetch spec will break any userland code that doesn't specifically catch timeouts.

@benjamingr
Copy link
Member

I feel strongly that core APIs that reject with AbortErrors shouldn't be overridable to reject with other stuff even if that's the .reason and should expose it as .cause instead. It means every user would need to refactor their error handling otherwise.

philipperolet added a commit to dust-tt/dust that referenced this issue May 6, 2024
Description
---
Those errors occur on the confluence connector when fetch is
aborted. Apparently a timeout can cause either an "AbortError" or a
"TimeoutError"

nodejs/node#43874
philipperolet added a commit to dust-tt/dust that referenced this issue May 6, 2024
Description
---
Those errors occur on the confluence connector when fetch is
aborted. Apparently a timeout can cause either an "AbortError" or a
"TimeoutError"

nodejs/node#43874
flvndvd pushed a commit to dust-tt/dust that referenced this issue May 26, 2024
Description
---
Those errors occur on the confluence connector when fetch is
aborted. Apparently a timeout can cause either an "AbortError" or a
"TimeoutError"

nodejs/node#43874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abortcontroller Issues and PRs related to the AbortController API fetch Issues and PRs related to the Fetch API
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants