Skip to content

Commit

Permalink
Fix teardown bug that was previously masked by memory leak.
Browse files Browse the repository at this point in the history
  • Loading branch information
kentonv committed Feb 1, 2024
1 parent e860ffe commit 1ad3252
Show file tree
Hide file tree
Showing 2 changed files with 18 additions and 8 deletions.
12 changes: 12 additions & 0 deletions src/workerd/api/streams/common.h
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,12 @@ class ReaderLocked {
return canceler;
}

void clear() {
reader = kj::none;
closedFulfiller = kj::none;
canceler = kj::none;
}

private:
kj::Maybe<ReadableStreamController::Reader&> reader;
kj::Maybe<jsg::Promise<void>::Resolver> closedFulfiller;
Expand Down Expand Up @@ -788,6 +794,12 @@ class WriterLocked {
}
}

void clear() {
writer = kj::none;
closedFulfiller = kj::none;
readyFulfiller = kj::none;
}

private:
kj::Maybe<WritableStreamController::Writer&> writer;
kj::Maybe<jsg::Promise<void>::Resolver> closedFulfiller;
Expand Down
14 changes: 6 additions & 8 deletions src/workerd/api/streams/internal.c++
Original file line number Diff line number Diff line change
Expand Up @@ -472,8 +472,7 @@ kj::Maybe<kj::Promise<DeferredProxy<void>>> WritableStreamSink::tryPumpFrom(
// =======================================================================================

ReadableStreamInternalController::~ReadableStreamInternalController() noexcept(false) {
KJ_IF_SOME(locked, readState.tryGet<ReaderLocked>()) {
auto lock = kj::mv(locked);
if (readState.is<ReaderLocked>()) {
readState.init<Unlocked>();
}
}
Expand Down Expand Up @@ -801,12 +800,12 @@ void ReadableStreamInternalController::releaseReader(
locked.getClosedFulfiller(),
js.v8TypeError("This ReadableStream reader has been released."_kj));
}
auto lock = kj::mv(locked);
locked.clear();

// When maybeJs is nullptr, that means releaseReader was called when the reader is
// being deconstructed and not as the result of explicitly calling releaseLock. In
// that case, we don't want to change the lock state itself because we do not have
// an isolate lock. Moving the lock above will free the lock state while keeping the
// an isolate lock. Clearing the lock above will free the lock state while keeping the
// ReadableStream marked as locked.
if (maybeJs != kj::none) {
readState.template init<Unlocked>();
Expand All @@ -815,8 +814,7 @@ void ReadableStreamInternalController::releaseReader(
}

WritableStreamInternalController::~WritableStreamInternalController() noexcept(false) {
KJ_IF_SOME(locked, writeState.tryGet<WriterLocked>()) {
auto lock = kj::mv(locked);
if (writeState.is<WriterLocked>()) {
writeState.init<Unlocked>();
}
}
Expand Down Expand Up @@ -1288,12 +1286,12 @@ void WritableStreamInternalController::releaseWriter(
locked.getClosedFulfiller(),
js.v8TypeError("This WritableStream writer has been released."_kj));
}
auto lock = kj::mv(locked);
locked.clear();

// When maybeJs is nullptr, that means releaseWriter was called when the writer is
// being deconstructed and not as the result of explicitly calling releaseLock and
// we do not have an isolate lock. In that case, we don't want to change the lock
// state itself. Moving the lock above will free the lock state while keeping the
// state itself. Clearing the lock above will free the lock state while keeping the
// WritableStream marked as locked.
if (maybeJs != kj::none) {
writeState.template init<Unlocked>();
Expand Down

0 comments on commit 1ad3252

Please sign in to comment.