denoland / deno

A modern runtime for JavaScript and TypeScript.
https://deno.com
MIT License
96.69k stars 5.34k forks source link

ReadableStream is missing asyncIterator and cancel does not cancel #19946

Open zbynekwinkler opened 1 year ago

zbynekwinkler commented 1 year ago

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 commented 1 year ago

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

sigmaSd commented 1 year ago

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

sigmaSd commented 1 year ago

There seem to be multiple bugs?,

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 commented 1 year ago

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

zbynekwinkler commented 1 year ago

@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 commented 1 year ago

The TextDecoderStream issue is the same as #13142

zbynekwinkler commented 1 year ago

@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 commented 11 months ago

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 commented 2 months ago

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