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

ReadableStream is missing asyncIterator and cancel does not cancel #19946

Open
zbynekwinkler opened this issue Jul 26, 2023 · 9 comments
Open

Comments

@zbynekwinkler
Copy link

Working with the example from https://deno.land/std@0.195.0/streams/text_line_stream.ts?s=TextLineStream:

import { TextLineStream } from "https://deno.land/std@0.195.0/streams/text_line_stream.ts";

Deno.test("a", async () => {
  const res = await fetch("https://example.com");
  const lines = res.body!
    .pipeThrough(new TextDecoderStream())
    .pipeThrough(new TextLineStream());

  for await (const line of lines) {
    console.log(line);
    break;
  }
  await lines.cancel();
});

Running this test with deno test results first in TS2504 error:

Check file:///.../tests/test.ts
error: TS2504 [ERROR]: Type 'ReadableStream<string>' must have a '[Symbol.asyncIterator]()' method that returns an async iterator.
  for await (const line of lines) {
                           ~~~~~
    at file:///.../test.ts:9:28

If I run it with --no-check I get a different error:

error: Leaking resources:
  - A text decoder (rid 6) was created during the test, but not finished during the test. Close the text decoder by calling `textDecoder.decode('')` or `await textDecoderStream.readable.cancel()`.

I am not sure if test is wrongly detecting a leak or if there is an actual leak but there does not seem to be anything else to do with the stream but cancel. It seems there is no error-free way out for this test to finish successfuly.

@zbynekwinkler
Copy link
Author

I am using deno 1.35.1. Running deno lint on the said file does not report any problem.

@sigmaSd
Copy link
Contributor

sigmaSd commented Jul 26, 2023

I can' t reproduce the asyniterator ts error, with deno1.35.2 linux

@sigmaSd
Copy link
Contributor

sigmaSd commented Jul 26, 2023

There seem to be multiple bugs?,

  • TextLineStream bug?: the above code only prints the first line , instead of all the lines
  • The leaking resource

iterating manually instead of the for await, fixes both issues

  const reader = lines.getReader();
  while (true) {
    const { done, value } = await reader.read();
    console.log(value);
    if (done) break;
  }

@bartlomieju
Copy link
Member

CC @marvinhagemeister IIRC, you hit the same problem recently. Any chance you could summarize what we discovered?

@zbynekwinkler
Copy link
Author

@sigmaSd Yes, there are possible workarounds. Not reading all the lines is intentional - it exposes the problem. I am not sure if there is an actual leak or if the test runner is miscomplaining. My understanding is that the cancel() should dismantle the stream so there should be no resource leak.

wrt async iterator - the thing is that I get the type check error with plain deno test -A but when I run it with --no-check it is not actually missing the iterator.

I've upgraded to 1.35.3 and I still get TS2504 error.

@marvinhagemeister I found this in

https://github.com/denoland/fresh/blob/efbff06fcaf30e8a5600d9fd299b75ed590c31b8/tests/test_utils.ts#L111

when the server outputs anything to stdout, I get the leak report.

@crowlKats
Copy link
Member

The TextDecoderStream issue is the same as #13142

@zbynekwinkler
Copy link
Author

@sigmaSd I was trying the workaround. Once the async iteration is started, reading the stream to the end (async or otherwise) does not help. So what I had to do was to replace https://github.com/denoland/fresh/blob/efbff06fcaf30e8a5600d9fd299b75ed590c31b8/tests/test_utils.ts#L130-L136 with the reader implementation. After that I deleted https://github.com/denoland/fresh/blob/efbff06fcaf30e8a5600d9fd299b75ed590c31b8/tests/test_utils.ts#L36 and added draining the stream after sending SIGTERM to the server process. That got rid off the leak.

I don't have enough understanding to say if #13142 is indeed a duplicate or not. What I am seeing is a leak if a started async iteration is not followed to the end. It might be the same thing if that counts as an "error", I don't know.

@hermy991
Copy link

The test worked for me

import { TextLineStream } from "https://deno.land/std@0.195.0/streams/text_line_stream.ts";

Deno.test("a", async () => {
  const res = await fetch("https://example.com");
  const lines = res.body!
    .pipeThrough(new TextDecoderStream())
    .pipeThrough(new TextLineStream());

  for await (const line of lines) {
    console.log(line);
    break;
  }
  await lines.cancel();
});

version:

deno 1.38.2 (release, x86_64-pc-windows-msvc)
v8 12.0.267.1
typescript 5.2.2

@raaymax
Copy link

raaymax commented Jul 28, 2024

I think my issue is connected to this one so I will paste it here.
I was able to narrow the bug down to the following code snippet:

Deno.test('stream iterator fail', async () => {
  const stream = new ReadableStream<Uint8Array>();
  const it = stream[Symbol.asyncIterator]();
  await it.next();
  await it.return?.();
  console.log('Stream closed');
});

Results with:

error: Promise resolution is still pending but the event loop has already resolved.

version:

deno 1.45.4 (release, aarch64-apple-darwin)
v8 12.7.224.13
typescript 5.5.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants