Skip to content

Commit 1db96e7

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 aab6dd1 commit 1db96e7

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
@@ -628,9 +628,7 @@ async function* createBlobReaderIterable(reader, options = kEmptyObject) {
628628
const { getReadError, maxBatchBytes = kDefaultMaxBatchBytes } = options;
629629
let wakeup = PromiseWithResolvers();
630630
let immediate;
631-
let fin = false;
632-
reader.setWakeup((setfin) => {
633-
fin ||= setfin;
631+
reader.setWakeup(() => {
634632
immediate ??= setImmediate(() => {
635633
immediate = undefined;
636634
wakeup.resolve?.();
@@ -690,12 +688,6 @@ async function* createBlobReaderIterable(reader, options = kEmptyObject) {
690688
if (blocked) {
691689
await wakeup.promise;
692690
wakeup = PromiseWithResolvers();
693-
// If the wakeup was triggered by FIN (EndReadable), the DataQueue
694-
// is capped. Continue the loop to pull again -- the next pull will
695-
// return EOS. Without this, a race between the data notification
696-
// and the FIN notification can leave the iterator waiting for a
697-
// wakeup that will never come.
698-
if (fin) continue;
699691
}
700692
}
701693
} 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
@@ -1648,10 +1648,9 @@ void Stream::EndReadable(std::optional<uint64_t> maybe_final_size) {
16481648
FlushAccumulation();
16491649
set_final_size(maybe_final_size.value_or(STAT_GET(Stats, bytes_received)));
16501650
inbound_->cap(STAT_GET(Stats, final_size));
1651-
// Notify the JS reader so it can see EOS. Pass fin=true so the
1652-
// wakeup promise resolves with a value the iterator can check to
1653-
// avoid waiting for another wakeup that will never come.
1654-
if (reader_) reader_->NotifyPull(true);
1651+
// Notify the JS reader so it can see EOS. The subsequent pull observes
1652+
// the now-capped DataQueue and returns EOS.
1653+
if (reader_) reader_->NotifyPull();
16551654
}
16561655

16571656
void Stream::Destroy(QuicError error) {

0 commit comments

Comments
 (0)