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

Invalid state: Controller is already closed #1564

Closed
FZambia opened this issue Jul 19, 2022 · 12 comments · Fixed by #1570
Closed

Invalid state: Controller is already closed #1564

FZambia opened this issue Jul 19, 2022 · 12 comments · Fixed by #1570
Labels
bug Something isn't working

Comments

@FZambia
Copy link

FZambia commented Jul 19, 2022

Bug Description

When running my tests with undici v5.8.0 I am getting the traceback:

node:internal/errors:466
    ErrorCaptureStackTrace(err);
    ^

TypeError: Invalid state: Controller is already closed
    at new NodeError (node:internal/errors:377:5)
    at ReadableStreamDefaultController.enqueue (node:internal/webstreams/readablestream:992:13)
    at Fetch.fetchParams.controller.resume (/Users/fz/projects/centrifugal/centrifuge-js/node_modules/undici/lib/fetch/index.js:1864:41)
    at processTicksAndRejections (node:internal/process/task_queues:95:5) {
  code: 'ERR_INVALID_STATE'

I did some research if this can be helpful:

In my case the code in undici is getting exception in calling await fetchParams.controller.next() in lib/fetch/index.js, so catch branch works:

      try {
        const { done, value } = await fetchParams.controller.next()

        if (isAborted(fetchParams)) {
          break
        }

        bytes = done ? undefined : value
      } catch (err) {
        if (fetchParams.controller.ended && !timingInfo.encodedBodySize) {
          // zlib doesn't like empty streams.
          bytes = undefined
        } else {
          bytes = err
        }
      }

I added some console logging:

      } catch (err) {
        console.log(err); // DOMException [AbortError]: The operation was aborted.
        console.log(fetchParams.controller.ended); // undefined
        console.log(timingInfo.encodedBodySize); // 136
        console.log(fetchParams.controller.ended && !timingInfo.encodedBodySize); // undefined
        if (fetchParams.controller.ended && !timingInfo.encodedBodySize) {
          // zlib doesn't like empty streams.
          bytes = undefined
        } else {
          console.log("bytes is err"); // bytes is err
          bytes = err
          console.log(bytes instanceof Error); // false
          console.log(typeof bytes); // object
        }
      }

The detailed exception is:

DOMException [AbortError]: The operation was aborted.
        at new DOMException (node:internal/per_context/domexception:51:5)
        at Fetch.abort (/Users/fz/projects/centrifugal/centrifuge-js/node_modules/undici/lib/fetch/index.js:89:20)
        at AbortSignal.requestObject.signal.addEventListener.once (/Users/fz/projects/centrifugal/centrifuge-js/node_modules/undici/lib/fetch/index.js:165:20)
        at AbortSignal.[nodejs.internal.kHybridDispatch] (node:internal/event_target:639:20)
        at AbortSignal.dispatchEvent (node:internal/event_target:581:26)
        at abortSignal (node:internal/abort_controller:291:10)
        at AbortController.abort (node:internal/abort_controller:321:5)
        at AbortSignal.abort (/Users/fz/projects/centrifugal/centrifuge-js/node_modules/undici/lib/fetch/request.js:372:32)
        at AbortSignal.[nodejs.internal.kHybridDispatch] (node:internal/event_target:639:20)
        at AbortSignal.dispatchEvent (node:internal/event_target:581:26)

So I suppose fetchParams.controller.controller.enqueue(new Uint8Array(bytes)) is called even if the stream was aborted.

Reproducible By

Create a new Node project with npm init

Add jest:

npm install --save-dev jest

Add test file index.test.js:

test('test exception', async () => {
    const d = new DOMException('test')
    expect(d).toBeInstanceOf(Error)
    expect(d instanceof Error).toBeTruthy()
})

Update package.json to have:

{
  "scripts": {
    "test": "jest"
  }
}

Run npm test

Expected Behavior

DOMException in Jest env does not extend Error, so stream aborting logic in

if (bytes instanceof Error) {
does not work.

Environment

Node v18.2.0

Running tests with jest

Additional context

This reproduces starting from undici v5.6.0

@FZambia FZambia added the bug Something isn't working label Jul 19, 2022
@mcollina
Copy link
Member

Thanks for reporting!

Can you provide steps to reproduce? We often need a reproducible example, e.g. some code that allows someone else to recreate your problem by just copying and pasting it. If it involves more than a couple of different file, create a new repository on GitHub and add a link to that.

@FZambia
Copy link
Author

FZambia commented Jul 19, 2022

Yep, I'll try to provide an example, not very simple to create a minimal one.

Think I found the PR which caused an issue in my case: #1511

@FZambia
Copy link
Author

FZambia commented Jul 19, 2022

So now I see that the reason is that when running tests with Jest:

test('test exception', async () => {
  console.log(globalThis.DOMException);
  const d = new globalThis.DOMException("");
  console.log(d instanceof Error);
})

DOMException instance is not an instance of Error, so

if (bytes instanceof Error) {

is not true.

But when simply running the same code with Node DOMException is an instance of Error. Previously (before undici v5.6.0 and #1511 in particular) there was custom AbortError that extended Error so my tests worked fine.

Do not understand at the moment whether this is Jest problem or Undici's, or my own, and why DOMException is not an instance of Error in Jest case.

@ronag
Copy link
Member

ronag commented Jul 19, 2022

We could make a isErrorLike helper which is a bit more forgiving. We've done this in other cases for increased interop.

@KhafraDev
Copy link
Member

KhafraDev commented Jul 19, 2022

This is an undici issue caused by #1511.

We still need a small repro for tests.

@FZambia
Copy link
Author

FZambia commented Jul 19, 2022

DOMException doesn't extend Error, even on the browser.

I tried in Chrome and Firefox:

> const d = new DOMException()
undefined
> d instanceof Error
true

Seems it's an instance of Error, or I am missing sth?

@KhafraDev
Copy link
Member

KhafraDev commented Jul 19, 2022

No, I was mistaken. However this seems more like a jest issue than an undici issue.

edit: If possible, you could try using vitest, which includes a similar api to jest. I've seen some projects move away from jest and it seems like a good alternative.

import { test, expect } from 'vitest' 

test('test exception', async () => {
  const d = new DOMException('')
  expect(d).toBeInstanceOf(Error)
  expect(d instanceof Error).toBeTruthy()
})

@FZambia
Copy link
Author

FZambia commented Jul 20, 2022

edit: If possible, you could try using vitest, which includes a similar api to jest. I've seen some projects move away from jest and it seems like a good alternative.

Thanks for a suggestion, jest has a nice --detectOpenHandles option which is really important for my use case, seems that vitest does not have it yet, so not sure I want to switch.

I don't see a workaround from my side for this, I can open an issue in Jest repo if you think it should be fixed there and link it to this one, or possibly you see a good way (like the one @ronag suggested above or handling DomException explicitly) to fix this from Undici's side?

@KhafraDev
Copy link
Member

Jest has had an issue with instanceof Error for over half a decade now, this will most likely need to be fixed here. I'll work on this tomorrow.

@FZambia
Copy link
Author

FZambia commented Jul 21, 2022

Many thanks for helping with this, I tried to run tests with undici from the latest commit and it worked fine.

@aralroca
Copy link

aralroca commented Sep 2, 2024

I have the same problem with Node 20. Switching to Node 22 is working fine, however my cloud provider is using the LTS (20) and I can't move to 22 there. Any workaround to solve it?

@KhafraDev
Copy link
Member

Stop using jest or use undici rather than the globals. Jest is bad.

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.

5 participants