Skip to content

fix(ext/web): fix promise chain leak of ReadableStream - #30871

Merged
bartlomieju merged 2 commits into
denoland:mainfrom
codepage949:fix-promise-chain-leak
Oct 13, 2025
Merged

bartlomieju merged 2 commits into
denoland:mainfrom
codepage949:fix-promise-chain-leak

Conversation

@codepage949

Copy link
Copy Markdown
Contributor

Closes #30298

This problem occurs because promise chain keep growing(like a stack overflow caused by a recursive function) while the stream is running. To solve this, used a loop.

@CLAassistant

CLAassistant commented Sep 29, 2025 •

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@bartlomieju
bartlomieju requested review from crowlKats and littledivy and removed request for crowlKats October 6, 2025 16:17

@littledivy littledivy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix!

Can you add a test in tests/unit/streams_test.ts that tries to trigger a stack overflow? A depth of 30_000 should be enough to reproduce.

@codepage949

Copy link
Copy Markdown
Contributor Author
Deno.test(async function readableStreamEmittingManyChunks() {
  const serverPort = 4594;
  const stopSignal = new AbortController();
  let count = 0;

  function startServer() {
    Deno.serve({ port: serverPort, signal: stopSignal.signal }, async (req) => {
      return new Response(
        new ReadableStream({
          pull(controller) {
            if (count < 30_000_000) {
              controller.enqueue(new Uint8Array([0]));
            } else {
              controller.close();
            }

            count += 1;
          },
        }),
      );
    });
  }

  async function startClient() {
    const response = await fetch(`http://localhost:${serverPort}`);
    const reader = response.body?.getReader();
    if (!reader) {
      throw new Error("client: failed to get reader from response");
    }

    while (true) {
      const { done } = await reader.read();
      if (done) break;
    }
  }

  startServer();
  await startClient();
  stopSignal.abort();
});

I used Deno.serve because the issue occurred when consuming a ReadableStream through Deno.serve.

I tried adjusting the depth using count, but in my environment, I had to specify a fairly large number to run out of memory. Results vary depending on the environment, and it takes a considerable amount of time, so it doesn't seem like a reliable test. Is there a better way?

@codepage949

Copy link
Copy Markdown
Contributor Author

I used a method to measure maximum heap memory usage. Since it seemed unstable to compare the results when mixed with other test tasks, I ran the task in a separate Deno instance. Before the modification, the ratio (after/before) was between 7 and 8, while after the modification, it was between 1.3 and 1.4. Therefore, I set the cutoff value at 1.5.

@bartlomieju bartlomieju left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks, this is a very good fix

@bartlomieju
bartlomieju merged commit 302b6fc into denoland:main Oct 13, 2025
19 checks passed
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

Successfully merging this pull request may close these issues.

It seems like using ReadableStream in Response causes a memory leak

4 participants