Skip to content

Commit 243905a

Browse files
authored
buffer: fix Blob.stream() leaking source buffer
Blob.prototype.stream() registered a wakeup callback on the underlying source's start() and never released it. The strong Reader::wakeup_ handle kept the reader -- and through it the blob's DataQueue and backing store -- reachable as a GC root, so the source buffer leaked on every stream() call. On Node 26+, streaming a 1 MiB blob 300 times retained ~300 MiB in process.memoryUsage().arrayBuffers while the V8 heap stayed small. Register the wakeup lazily in pull() and clear it on every terminal or idle path (EOS, error, cancel, backpressure), mirroring the cleanup already done by the async iterator path. The strong handle now only lives while a pull is in flight, so the reader and its backing store become collectable once the stream finishes, errors, is cancelled, or goes idle under backpressure. Fixes: #63574 Signed-off-by: semimikoh <ejffjeosms@gmail.com> PR-URL: #63577 Fixes: #63574 Reviewed-By: James M Snell <jasnell@gmail.com>
1 parent 0032189 commit 243905a

2 files changed

Lines changed: 73 additions & 4 deletions

File tree

lib/internal/blob.js

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -473,30 +473,40 @@ function createBlobReaderStream(reader) {
473473
// There really should only be one read at a time so using an
474474
// array here is purely defensive.
475475
this.pendingPulls = [];
476-
// Register a wakeup callback that the C++ side can invoke
476+
// Lazily register a wakeup callback that the C++ side can invoke
477477
// when new data is available after a STATUS_BLOCK.
478-
reader.setWakeup(() => {
478+
this.wakeup = () => {
479479
if (this.pendingPulls.length > 0) {
480480
this.readNext(c);
481481
}
482-
});
482+
};
483483
},
484484
pull(c) {
485485
const { promise, resolve, reject } = PromiseWithResolvers();
486+
if (this.pendingPulls.length === 0) {
487+
reader.setWakeup(this.wakeup);
488+
}
486489
this.pendingPulls.push({ resolve, reject });
487490
this.readNext(c);
488491
return promise;
489492
},
493+
clearWakeupIfIdle() {
494+
if (this.pendingPulls.length === 0) {
495+
reader.setWakeup(undefined);
496+
}
497+
},
490498
readNext(c) {
491499
reader.pull((status, buffer) => {
492500
// If pendingPulls is empty here, the stream had to have
493501
// been canceled, and we don't really care about the result.
494502
// We can simply exit.
495503
if (this.pendingPulls.length === 0) {
504+
reader.setWakeup(undefined);
496505
return;
497506
}
498507
if (status === 0) {
499508
// EOS
509+
reader.setWakeup(undefined);
500510
c.close();
501511
// This is to signal the end for byob readers
502512
// see https://streams.spec.whatwg.org/#example-rbs-pull
@@ -508,6 +518,7 @@ function createBlobReaderStream(reader) {
508518
// The read could fail for many different reasons when reading
509519
// from a non-memory resident blob part (e.g. file-backed blob).
510520
// The error details the system error code.
521+
reader.setWakeup(undefined);
511522
const error =
512523
lazyDOMException('The blob could not be read',
513524
'NotReadableError');
@@ -517,7 +528,7 @@ function createBlobReaderStream(reader) {
517528
return;
518529
} else if (status === 2) {
519530
// STATUS_BLOCK: No data available yet. The wakeup callback
520-
// registered in start() will re-invoke readNext when data
531+
// registered in pull() will re-invoke readNext when data
521532
// arrives.
522533
return;
523534
}
@@ -537,6 +548,7 @@ function createBlobReaderStream(reader) {
537548
if (this.pendingPulls.length !== 0) {
538549
const pending = this.pendingPulls.shift();
539550
pending.resolve();
551+
this.clearWakeupIfIdle();
540552
}
541553
return;
542554
}
@@ -545,6 +557,7 @@ function createBlobReaderStream(reader) {
545557
});
546558
},
547559
cancel(reason) {
560+
reader.setWakeup(undefined);
548561
// Reject any currently pending pulls here.
549562
for (const pending of this.pendingPulls) {
550563
pending.reject(reason);
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
// Flags: --expose-gc --no-concurrent-array-buffer-sweeping
2+
'use strict';
3+
4+
const common = require('../common');
5+
const assert = require('assert');
6+
const { setImmediate: setImmediatePromise } = require('timers/promises');
7+
8+
const MiB = 1024 * 1024;
9+
const iterations = 64;
10+
const maxRetained = 16 * MiB;
11+
12+
async function collectArrayBuffers() {
13+
for (let i = 0; i < 3; i++) {
14+
global.gc();
15+
await setImmediatePromise();
16+
}
17+
}
18+
19+
async function assertNoBlobStreamRetention(name, fn) {
20+
const buffer = Buffer.alloc(MiB);
21+
22+
await collectArrayBuffers();
23+
const before = process.memoryUsage().arrayBuffers;
24+
25+
for (let i = 0; i < iterations; i++) {
26+
await fn(buffer);
27+
}
28+
29+
await collectArrayBuffers();
30+
const retained = process.memoryUsage().arrayBuffers - before;
31+
32+
assert(
33+
retained < maxRetained,
34+
`${name} retained ${retained} bytes in arrayBuffers`,
35+
);
36+
}
37+
38+
(async () => {
39+
await assertNoBlobStreamRetention('unused Blob streams',
40+
common.mustCall(async (buffer) => {
41+
new Blob([buffer]).stream();
42+
}, iterations));
43+
44+
await assertNoBlobStreamRetention('cancelled Blob streams',
45+
common.mustCall(async (buffer) => {
46+
await new Blob([buffer]).stream()
47+
.cancel();
48+
}, iterations));
49+
50+
await assertNoBlobStreamRetention('drained Blob streams',
51+
common.mustCall(async (buffer) => {
52+
await new Response(
53+
new Blob([buffer]).stream(),
54+
).arrayBuffer();
55+
}, iterations));
56+
})().then(common.mustCall());

0 commit comments

Comments
 (0)