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

Race condition w/ body cancel #2009

Closed
ethanresnick opened this issue Mar 16, 2023 · 0 comments · Fixed by #2013
Closed

Race condition w/ body cancel #2009

ethanresnick opened this issue Mar 16, 2023 · 0 comments · Fixed by #2013
Assignees
Labels
bug Something isn't working fetch

Comments

@ethanresnick
Copy link

ethanresnick commented Mar 16, 2023

Bug Description

Calling response.body.cancel — but only with a reason message — can lead to controller already closed errors.

Reproducible By

import { fetch } from "undici";

await fetch("https://httpbin.org", {
  method: "POST",
  body: "{}",
  headers: { "Content-Type": "application/json" },
}).then(
  async (resp) => {
    await resp.body.cancel("Some message");
  },
  (err) => {}
);

Expected Behavior

HTTPBin returns a 405 with an HTML body for this request. Calling resp.body.cancel(reasonString) should not error.

Logs & Screenshots

About 80% of the time, the code above gives the following (uncaught) rejection:

TypeError [ERR_INVALID_STATE]: Invalid state: Controller is already closed
    at new NodeError (node:internal/errors:399:5)
    at ReadableStreamDefaultController.enqueue (node:internal/webstreams/readablestream:1036:13)
    at fetchParams.controller.resume (/Users/ethanresnick/Desktop/node_modules/undici/lib/fetch/index.js:1888:41)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5) {
  code: 'ERR_INVALID_STATE'

It seems like there might be some sort of race condition.

Interestingly, if the code calls resp.body.cancel() (i.e., leaving the reason string out), the code never seems to throw. I think it may be that, somehow, the string passed to .cancel() is propagated to here, where it fails the isErrorLike check... or at least that would be consistent with this earlier bug: #1564

Perhaps, rather than solely relying on an isErrorLike check on the bytes, it'd be good to set a flag if this line was reached, and check if isErrorLIike(bytes) || hitCatchFlag.

Environment

MacOS 13.2
Nodejs 19.6.0
undici 5.21.0

Additional context

In our code, we call other origins sometimes (i.e., not HTTPBin) and discard the body, and this issue doesn't seem to occur, so there may be some weird interaction going on between undici and httpbin. For example, the following code will never throw, even though this origin also returns 405 (with a plaintext body):

await fetch("https://protegoapi.com", {
  method: "POST",
  body: "{}",
  headers: { "Content-Type": "application/json" },
}).then(
  async (resp) => {
    await resp.body.cancel("Reason");
  },
  (err) => {}
);
@ethanresnick ethanresnick added the bug Something isn't working label Mar 16, 2023
@ethanresnick ethanresnick changed the title Race condition w/ body cancel, and strange mixing results Race condition w/ body cancel Mar 16, 2023
@KhafraDev KhafraDev self-assigned this Mar 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fetch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants