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

Unable to completely read data from 'bytes' ReadableStream #48233

Closed
debadree25 opened this issue May 28, 2023 · 6 comments
Closed

Unable to completely read data from 'bytes' ReadableStream #48233

debadree25 opened this issue May 28, 2023 · 6 comments

Comments

@debadree25
Copy link
Member

debadree25 commented May 28, 2023

Version

20.2.0

Platform

Darwin Debadree-MacBook-Pro.local 22.4.0 Darwin Kernel Version 22.4.0: Mon Mar 6 21:00:17 PST 2023; root:xnu-8796.101.5~3/RELEASE_X86_64 x86_64

Subsystem

Web streams

What steps will reproduce the bug?

Run the following script in node and in chrome

const toPull = ['a', 'b', 'c', 'd', 'e', 'f', 'g', 'h', 'i', 'j'];
const st = new ReadableStream({
  type: 'bytes',
  start(controller) {},

  pull(controller) {
    const chunk = toPull.shift();
    if (chunk === undefined) {
      controller.close();
      return;
    }

    controller.enqueue(new TextEncoder().encode(chunk));
  },
});

const reader = st.getReader({ mode: 'byob' });
const chunks = [];

const decoder = new TextDecoder();

let result;
do {
  result = await reader.read(new Uint8Array(100));
  if (result.value !== undefined)
    chunks.push(decoder.decode(result.value));
} while (!result.done);

console.log(chunks);

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

Always

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

[
  'a', 'b', 'c', 'd',
  'e', 'f', 'g', 'h',
  'i', 'j'
]

The stream should consume all the elements in the toPull array and we must be able to subsequently read this data back, the behaviour is correct in chrome.

What do you see instead?

No output

debadreechatterjee@Debadree-MacBook-Pro node % node test3.mjs
debadreechatterjee@Debadree-MacBook-Pro node % 

Additional information

I tried investigating a little, its possible that there is some hanging promise before the closing of the stream. for example if you logged the values in each step the output would be like this

debadreechatterjee@Debadree-MacBook-Pro node % node test3.mjs
read value a
read value b
read value c
read value d
read value e
read value f
read value g
read value h
read value i
read value j
debadreechatterjee@Debadree-MacBook-Pro node %

note that console to print the chunks array is not printed

@debadree25 debadree25 changed the title Promise hang in 'bytes' ReadableStream Unable to completely read data from 'bytes' ReadableStream May 28, 2023
@debadree25
Copy link
Member Author

Also cc @nodejs/whatwg-stream

@Linkgoron
Copy link
Member

Linkgoron commented Jun 8, 2023

I found the issue and have a fix, I'll create a PR later.

From what I've found, readableStreamClose incorrectly handles only StreamDefaultReader correctly and not BYOB. If you look at readableStreamError in comparison it handles both BYOB and default.

Linkgoron added a commit to Linkgoron/node that referenced this issue Jun 8, 2023
Fix reader.read promise not fulfilling when the controller is closed
when the reader is a BYOBReader

fixes: nodejs#48233
@Linkgoron
Copy link
Member

Linkgoron commented Jun 9, 2023

I take back my previous statement... I think I understand what the issue is, and I managed to create a fix (at least it passed both wpt tests and Node's own tests) - however as I understand it - the implementation is directly from the spec. I checked and this also reproduces on Deno as well.

@debadree25
Copy link
Member Author

Thats quite weird
is the spec wrong or does chrome deviate from the spec?

@Linkgoron
Copy link
Member

Thats quite weird is the spec wrong or does chrome deviate from the spec?

I just tried executing your code in the devtools, and I see the same behavior as in Node and Deno.

Simpler example:

const st = new ReadableStream({
  type: 'bytes',
  start(controller) {},

  pull(controller) {
    controller.close();
  },
});

const reader = st.getReader({ mode: 'byob' });
const decoder = new TextDecoder();
console.log('before')
result = await reader.read(new Uint8Array(100));
console.log('after');

@debadree25
Copy link
Member Author

This is indeed spec compliant behvaiour in order to end byob streams we have to send a controller.byobRequest.respond(0); ref: https://streams.spec.whatwg.org/#example-rbs-pull

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