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

WebStream adapter throws unhandleable error #54205

Closed
indutny opened this issue Aug 4, 2024 · 0 comments · Fixed by #54206
Closed

WebStream adapter throws unhandleable error #54205

indutny opened this issue Aug 4, 2024 · 0 comments · Fixed by #54206

Comments

@indutny
Copy link
Member

indutny commented Aug 4, 2024

Version

v20.16.0

Platform

23.5.0 Darwin Kernel Version 23.5.0: Wed May  1 20:19:05 PDT 2024; root:xnu-10063.121.3~5/RELEASE_ARM64_T8112 arm64

Subsystem

stream

What steps will reproduce the bug?

Two separate test cases:

const { Readable } = require('node:stream');

const r = Readable.from(['data']);

const wrapper = Readable.fromWeb(Readable.toWeb(r));

wrapper.on('data', () => {
  wrapper.destroy();
});

and

const { Readable } = require('node:stream');

const r = new Readable({
  read() {
    this.push(null);
  },
});

r.on('close', () => reader.cancel());

const reader = Readable.toWeb(r).getReader();
reader.read();

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

100% reproducible using script above

What is the expected behavior? Why is that the expected behavior?

An exception should not be thrown.

What do you see instead?

node:internal/webstreams/readablestream:1042
      throw new ERR_INVALID_STATE.TypeError('Controller is already closed');
      ^

TypeError [ERR_INVALID_STATE]: Invalid state: Controller is already closed
    at ReadableStreamDefaultController.close (node:internal/webstreams/readablestream:1042:13)
    at Readable.<anonymous> (node:internal/webstreams/adapters:451:16)
    at Readable.<anonymous> (node:internal/util:525:20)
    at Readable.onclose (node:internal/streams/end-of-stream:162:14)
    at Readable.emit (node:events:531:35)
    at emitCloseNT (node:internal/streams/destroy:147:10)
    at process.processTicksAndRejections (node:internal/process/task_queues:81:21) {
  code: 'ERR_INVALID_STATE'
}

And there doesn't seem to be a way to catch this exception.

Additional information

No response

indutny added a commit to indutny/io.js that referenced this issue Aug 4, 2024
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: nodejs#54205
targos pushed a commit that referenced this issue Aug 14, 2024
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: #54205
PR-URL: #54206
Fixes: #54205
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this issue Sep 21, 2024
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: #54205
PR-URL: #54206
Fixes: #54205
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this issue Sep 26, 2024
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: #54205
PR-URL: #54206
Fixes: #54205
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
targos pushed a commit that referenced this issue Oct 2, 2024
WebStream's Readable controller does not tolerate `.close()` being
called after an `error`. However, when wrapping a Node's Readable stream
it is possible that the sequence of events leads to `finished()`'s
callback being invoked after such `error`.

In order to handle this, in this change we call the `finished()` handler
earlier when controller is canceled, and always handle this as an error
case.

Fix: #54205
PR-URL: #54206
Fixes: #54205
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Mattias Buelens <mattias@buelens.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants