Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/deno/src/utils/streaming.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,10 @@ function monitorStream(
onDone: () => void,
): ReadableStream<Uint8Array<ArrayBufferLike>> {
const reader = stream.getReader();
// oxlint-disable-next-line typescript/no-floating-promises
reader.closed.finally(() => onDone());
reader.closed.then(
() => onDone(),
() => onDone(),
);
return new ReadableStream({
async start(controller) {
let result: ReadableStreamReadResult<Uint8Array<ArrayBufferLike>>;
Expand Down
56 changes: 56 additions & 0 deletions packages/deno/test/streaming.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
// <reference lib="deno.ns" />

import { assertEquals } from 'https://deno.land/std@0.212.0/assert/mod.ts';

Deno.test('reader.closed.then(f, f) suppresses rejection when releaseLock is called on an open stream', async () => {
// Reproduces the bug from GitHub issue #20177:
// In monitorStream, reader.releaseLock() is called while the source stream
// is still open (e.g. the error path when controller.enqueue() throws).
// Per WHATWG Streams spec, this rejects reader.closed with a TypeError.
// Using .then(onDone, onDone) handles both cases; .finally() would propagate
// the rejection as unhandled.

let onDoneCalled = false;

const stream = new ReadableStream<Uint8Array>({
start(controller) {
controller.enqueue(new TextEncoder().encode('data'));
// intentionally not closing — stream stays open
},
});

const reader = stream.getReader();

// This is the exact pattern from monitorStream (line 84 in streaming.ts).
// With .finally(() => onDone()), this would propagate the rejection.
reader.closed.then(
() => {
onDoneCalled = true;
},
() => {
onDoneCalled = true;
},
);

await reader.read();

// This is what monitorStream does on the error path (line 98) when
// controller.enqueue() throws — releaseLock while the source is still open.
reader.releaseLock();

let unhandledRejection: PromiseRejectionEvent | undefined;
const handler = (e: PromiseRejectionEvent): void => {
e.preventDefault();
unhandledRejection = e;
};
globalThis.addEventListener('unhandledrejection', handler);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unhandled rejection listener registered after triggering action

Low Severity

The unhandledrejection event listener is registered on line 46, after reader.releaseLock() on line 39 which is the action that would trigger the rejection. If the fix were ever reverted to .finally(), the unhandled rejection event could fire before the listener is attached, causing the assertEquals(unhandledRejection, undefined, ...) assertion to pass incorrectly — hiding the regression the test is meant to catch. The listener needs to be set up before reader.releaseLock() to reliably detect unhandled rejections. Also, the test uses setTimeout(resolve, 50) which is a sleep-in-test pattern; a more deterministic signal would be preferable.

Fix in Cursor Fix in Web

Triggered by project rule: PR Review Guidelines for Cursor Bot

Reviewed by Cursor Bugbot for commit 7f65c39. Configure here.


try {
await new Promise(resolve => setTimeout(resolve, 50));

assertEquals(onDoneCalled, true, 'onDone should have been called via the rejection handler');
assertEquals(unhandledRejection, undefined, 'should not have caused an unhandled promise rejection');
} finally {
globalThis.removeEventListener('unhandledrejection', handler);
}
});
Loading