Skip to content

Commit 024bef2

Browse files
trivenayaduh95
authored andcommitted
quic: remove unused fin flag from blob reader wakeup
The fin argument threaded from Blob::Reader::NotifyPull to the JS blob reader iterator was dead: the only consumer, `if (fin) continue;`, was the last statement in the loop and behaved identically to falling through. End-of-stream is always discovered by the subsequent pull returning EOS, never via the wakeup label. Remove the flag from the JS iterator, NotifyPull's signature/argv, and the EndReadable call site. The `!fin` coalescing bypass collapses safely because a parked reader always has pull_pending_ == false, so the first wakeup after parking always fires. Refs: #64767 Signed-off-by: Naman Trivedi <trivenay@amazon.com> PR-URL: #65315 Reviewed-By: Tim Perry <pimterry@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
1 parent 191ebfb commit 024bef2

4 files changed

Lines changed: 11 additions & 24 deletions

File tree

lib/internal/blob.js

Lines changed: 1 addition & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -576,9 +576,7 @@ async function* createBlobReaderIterable(reader, options = {}) {
576576
const { getReadError } = options;
577577
let wakeup = PromiseWithResolvers();
578578
let immediate;
579-
let fin = false;
580-
reader.setWakeup((setfin) => {
581-
fin ||= setfin;
579+
reader.setWakeup(() => {
582580
immediate ??= setImmediate(() => {
583581
immediate = undefined;
584582
wakeup.resolve?.();
@@ -630,12 +628,6 @@ async function* createBlobReaderIterable(reader, options = {}) {
630628
if (blocked) {
631629
await wakeup.promise;
632630
wakeup = PromiseWithResolvers();
633-
// If the wakeup was triggered by FIN (EndReadable), the DataQueue
634-
// is capped. Continue the loop to pull again -- the next pull will
635-
// return EOS. Without this, a race between the data notification
636-
// and the FIN notification can leave the iterator waiting for a
637-
// wakeup that will never come.
638-
if (fin) continue;
639631
}
640632
}
641633
} finally {

src/node_blob.cc

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -423,20 +423,16 @@ void Blob::Reader::SetWakeup(const FunctionCallbackInfo<Value>& args) {
423423
reader->wakeup_.Reset(args.GetIsolate(), args[0].As<Function>());
424424
}
425425

426-
void Blob::Reader::NotifyPull(bool fin) {
426+
void Blob::Reader::NotifyPull() {
427427
if (wakeup_.IsEmpty() || !env()->can_call_into_js()) return;
428-
// FIN notifications always fire — they must not be suppressed by
429-
// pull_pending_ because there will be no further notifications to
430-
// wake the iterator. Regular data notifications respect pull_pending_
431-
// to coalesce multiple deliveries within a single packet.
432-
if (!fin && pull_pending_) return;
428+
// Coalesce notifications: if a wakeup is already pending and the reader
429+
// has not yet pulled, skip re-notifying to avoid redundant wakeups
430+
// within a single packet.
431+
if (pull_pending_) return;
433432
pull_pending_ = true;
434433
HandleScope handle_scope(env()->isolate());
435434
Local<Function> fn = wakeup_.Get(env()->isolate());
436-
// Pass fin as the first argument so the JS iterator knows EOS is
437-
// imminent and should pull again without waiting for another wakeup.
438-
Local<Value> argv[] = {v8::Boolean::New(env()->isolate(), fin)};
439-
MakeCallback(fn, 1, argv);
435+
MakeCallback(fn, 0, nullptr);
440436
}
441437

442438
BaseObjectPtr<BaseObject> Blob::BlobTransferData::Deserialize(

src/node_blob.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -82,7 +82,7 @@ class Blob : public BaseObject {
8282
BaseObjectPtr<Blob> blob);
8383
static void Pull(const v8::FunctionCallbackInfo<v8::Value>& args);
8484
static void SetWakeup(const v8::FunctionCallbackInfo<v8::Value>& args);
85-
void NotifyPull(bool fin = false);
85+
void NotifyPull();
8686

8787
explicit Reader(Environment* env,
8888
v8::Local<v8::Object> obj,

src/quic/streams.cc

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1613,10 +1613,9 @@ void Stream::EndReadable(std::optional<uint64_t> maybe_final_size) {
16131613
FlushAccumulation();
16141614
set_final_size(maybe_final_size.value_or(STAT_GET(Stats, bytes_received)));
16151615
inbound_->cap(STAT_GET(Stats, final_size));
1616-
// Notify the JS reader so it can see EOS. Pass fin=true so the
1617-
// wakeup promise resolves with a value the iterator can check to
1618-
// avoid waiting for another wakeup that will never come.
1619-
if (reader_) reader_->NotifyPull(true);
1616+
// Notify the JS reader so it can see EOS. The subsequent pull observes
1617+
// the now-capped DataQueue and returns EOS.
1618+
if (reader_) reader_->NotifyPull();
16201619
}
16211620

16221621
void Stream::Destroy(QuicError error) {

0 commit comments

Comments
 (0)