Repository navigation
Add jsg::TracedOwn so visited handles survive their owner moving - #8103
littledivy wants to merge 3 commits into
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8103 +/- ##
==========================================
+ Coverage 68.31% 68.88% +0.57%
==========================================
Files 726 737 +11
Lines 189670 194137 +4467
Branches 29672 30118 +446
==========================================
+ Hits 129573 133739 +4166
+ Misses 42721 42720 -1
- Partials 17376 17678 +302 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
eb71e32 to
652980e
Compare
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
Superseded by the review of
0d8d7ef4.
This review, of 652980ec
💬 Approved with comments — No blocking or warning-level defects found. The design holds up on reading, and the queue visits are behind a default-off autogate. What remains is one question about pending reads visited by two parents, plus minor notes on lock assertions, formatting and docs.
Findings: 1 question · 2 info · 1 suggestion
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: ❓ 1 question · 1 info · 1 suggestion
The core design holds up. One question about how two visiting parents interact, and two minor notes.
Recommendation: 💬 Optional Nits
- [QUESTION] Pending reads are visited by both QueueImpl and ConsumerImpl, and handle strength follows whichever parent visited last. If one parent is strong from C++ with no wrapper and the other is only traced, can a traced-last visit let the resolver be freed while the strong side can still settle it? If that can't happen, please say so in the comment. (
src/workerd/api/streams/queue.h:265) - [INFO] TracedOwn's noexcept move and release() call into V8 after a visit. The isolate-lock requirement is checked only by a KJ_DASSERT, which release builds drop. Consider a KJ_ASSERT in TracedOwn::makeStrong. (
src/workerd/jsg/jsg.h:2434) - [SUGGESTION] The header comment in traced-own.h has a 140-column line and uneven wrapping;
just formatwill reflow it. (tools/clang-tidy/traced-own.h:14)
View full review
Checked and found correct
- With the gate off,
TracedOwnbehaves likekj::Own.makeStrong()checks the autogate, and bothQueueImpl::visitForGcandConsumerImpl::visitForGcskip the pending-read visits. - When
Wrappable::visitRefmakes a ref strong, it clearsrefParentbefore another visitor can reach the&p == &parentassertion. - The visit sites outside
jsg/that went through akj::Ownare converted. The one remaining site, intrace.h, has aNOLINTwith a reason. - The gate-on and gate-off wd-tests cover the gated behaviour.
Question: pending reads visited by two parents (src/workerd/api/streams/queue.h:265)
A pending read's handles are now visited by two parents: QueueImpl through the controller, and ConsumerImpl through the stream. GcVisitor::visit(Data&) sets the handle's strength to match whichever parent visited last. The comment covers the case where both parents have wrappers. Now suppose one parent is held strongly from C++ and has no wrapper, while the other is only traced. The handle then switches between strong and traced depending on visit order. If the traced parent visits last and its wrapper later becomes unreachable, the resolver could be freed while the strongly held side can still settle it. Can that combination happen, for example with streams created natively in C++? If it can't, saying so in the comment would help. The same applies to TracedOwn::visitedIn, which records only the last visit.
Lock requirement on moves (src/workerd/jsg/jsg.h)
After a visit, the noexcept move constructor, move assignment and release() can call into V8 (ClearWeak, addStrongRef). The isolate-lock requirement is checked only by a KJ_DASSERT, which is compiled out of release builds. Today's call sites all run under the lock, but a KJ_ASSERT in TracedOwn::makeStrong would make a future misuse fail loudly instead of quietly corrupting handle state.
Formatting (tools/clang-tidy/traced-own.h)
The header comment has a 140-column line and uneven wrapping. just format will reflow it.
📖 AGENTS.md: 💬 1 info
The new clang-tidy check is not listed in the build guidance.
Recommendation: 💬 Optional Nits
- [INFO] Add the new
jsg-traced-ownclang-tidy check to the check list in build/AGENTS.md. (tools/clang-tidy/workerd-lint.c++:14)
View full review
tools/clang-tidy/workerd-lint.c++ registers jsg-traced-own, but the clang-tidy check list in build/AGENTS.md doesn't mention it. A short entry with its test target would help contributors find it.
✅ All other checks passed
🔒 Security: ✅ LGTM
This is a GC and handle-lifetime change. It adds no input handling, authentication, secrets or injection surface.
⚡ Performance: ✅ LGTM
The new pending-read visits cost time linear in the number of pending reads and are behind the gc-complete-tracing autogate. The new makeStrong branches in Wrappable::visitRef and GcVisitor::visit are constant-time checks. The change removes a leak of streams whose read never settles.
📝 Documentation: ✅ LGTM
The README updates in jsg/ and api/streams/ match the code.
Reviewed commit: 652980ec
652980e to
0d8d7ef
Compare
There was a problem hiding this comment.
Superseded by the review of
612c95ac.
This review, of 0d8d7ef4
💬 Approved with comments — Every point from the last review is fixed at this commit. One INFO note remains: the READMEs don't mention the autogate condition on TracedOwn's move behaviour.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📝 Documentation: 💬 1 info
The READMEs don't mention that TracedOwn's move behaviour depends on the autogate.
Recommendation: 💬 Optional Nits
- [INFO] The README explanation of TracedOwn doesn't say that making handles strong on move depends on the gc-complete-tracing autogate. With the gate disabled, TracedOwn behaves like kj::Own (see jsg.h:2370). Consider adding a note here and in api/streams/README.md. (
src/workerd/jsg/README.md:259)
View full review
src/workerd/jsg/README.md and src/workerd/api/streams/README.md say a TracedOwn makes its handles strong when it moves. That only happens with the gc-complete-tracing autogate enabled; with it disabled, TracedOwn behaves like kj::Own. The header comment at src/workerd/jsg/jsg.h:2370 already explains this. A short note in the READMEs would tell contributors that the protection is off until the gate is on.
✅ All other checks passed
🔒 Security: ✅ LGTM
This is a GC and handle-lifetime change. It adds no input handling, authentication, secrets or injection surface.
📖 AGENTS.md: ✅ LGTM
build/AGENTS.md now lists the jsg-traced-own check with its test target.
📋 Code Quality: ✅ LGTM
- Pending reads visited by two parents: fixed. Only
QueueImpl::visitForGcvisits pending reads now, throughvisitPendingReads.makePendingReadsStrong()inQueueImpl::close()anderror()makes them strong before the controller stops visiting.TracedOwn::visitForGcnow records a strong visit whenkeepsHandlesStrong()is true. - Isolate lock:
TracedOwn::makeStrongnow usesKJ_ASSERT(v8::Locker::IsLocked(...)). - Header formatting: the comment in
tools/clang-tidy/traced-own.his rewrapped.
Every path that takes a read request out of the ring buffer (queue.c++:402, the extractPendingReads callers, ConsumerImpl::close) moves a TracedOwn, so the move makes its handles strong. The new GC tests cover close and error settling across tee branches, and a live controller keeping a pending read alive.
⚡ Performance: ✅ LGTM
The pending-read visits cost time linear in the number of pending reads and are behind the gc-complete-tracing autogate. The new makeStrong branches are constant-time checks.
Reviewed commit: 0d8d7ef4
A visited handle is traced: only its owner's next visit keeps the target alive. Moving a jsg::Data makes the destination strong again, but moving the kj::Own<T> of an object that holds handles moves only the pointer. The handles stay traced with nothing visiting them, and the GC can free their targets while the object still uses them. Code that hit this either left the handles unvisited, which leaks, or worked around it by hand. The stream queue did the first: it never visited the resolvers of pending reads, so a ReadableStream whose read never settles was never collected, along with its reader and everything they held. WritableStreamInternalController::drain() does the second (see 687b3a6). In this patch: - GcVisitor::makeStrong() makes every handle reachable through a value strong: jsg::Data, the Rust Global/Traced pair, and Refs, whose parent it clears so another owner can visit them. It matches make_strong in Rust JSG's jsg::Cell (#7982), so both languages follow one rule: whatever leaves a traced owner is strong. - jsg::TracedOwn<T> is an owning pointer that records the isolate of its last visit and strengthens the object when it moves or is released. MemoryTracker accepts it like kj::Own. - The stream queue holds pending reads in TracedOwn and visits them, along with a BYOB read's view, from the queue alone: a handle visited from two parents is strong or traced depending on which visited last. Every stream holds its controller, so the queue's visit keeps a pending read alive while its stream or its controller is reachable, as in the spec, where the controller holds its stream. close() and error() make the reads strong before the queue stops being visited, and cancel() drains the ring one element at a time instead of moving it. - The visits, and TracedOwn making handles strong, are behind a new autogate, gc-complete-tracing. With it disabled the resolvers stay unvisited strong roots and TracedOwn behaves like kj::Own, as before, so the rollout can be compared gated against ungated and turned off if code turns out to rely on such a stream staying alive. - A new clang-tidy check, jsg-traced-own, flags a visitForGc(), or a helper handed the visitor, that visits through a kj::Own, and kj::mv() of a container field whose elements hold a TracedOwn, directly or in a struct, kj::Maybe or kj::OneOf. Every site it found now holds a TracedOwn: stream readables and their consumers, the internal stream's pipe events, EventTarget listeners, CryptoKey and WebSocket. None of them moves its Own today; the check keeps it that way. The trace Request's Detail is shared through kj::addRef, so it keeps its kj::Own and the site is suppressed. - The jsg README, AGENTS.md and the GcVisitor comment describe the rule. Shared ownership is not covered: queue buffer entries are kj::Rc shared between consumers, so a buffered chunk that refers to its own stream still keeps the stream alive. Tests: - tracing-test: a value and a Ref taken out of a traced queue survive the queue being collected, and a cycle through a queued item is collected. With strengthening disabled the first case fails ("a value taken out of a traced queue was collected"). - streams-pending-gc-test, with the autogate enabled: streams, readers and BYOB streams with a pending read are collected; a pending read still settles, including when only the controller is reachable and on both branches of a tee when the stream closes. streams-pending-gc-gate-off-test runs the same cases with the autogate disabled and checks that none of these streams is collected, as on main. - traced-own-test covers the clang-tidy check, memory-test the MemoryTracker overload, and standard-test detaching a readable stream now held in a TracedOwn. 1,000 streams with a never-settling read, after GC: 0 collected with the autogate disabled, 999 with it enabled. RSS of `workerd serve` for a worker whose every request leaves one such stream behind (macOS, fastbuild): requests disabled enabled 20,000 246 MB 246 MB 60,000 482 MB 261 MB 100,000 801 MB 261 MB With the autogate disabled, RSS grows like on main; with it enabled, it levels off.
0d8d7ef to
612c95a
Compare
There was a problem hiding this comment.
Superseded by the review of
1d4f90eb.
This review, of 612c95ac
✅ Approved — The last open note, about the READMEs not mentioning the autogate condition, is fixed. The README updates since the last review raise no new issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
This is a GC and handle-lifetime change. It adds no input handling, authentication, secrets or injection surface.
📖 AGENTS.md: ✅ LGTM
build/AGENTS.md lists the jsg-traced-own check with its test target. src/workerd/jsg/AGENTS.md documents the TracedOwn rule.
📋 Code Quality: ✅ LGTM
No new issues. Every earlier code-quality point stays fixed at this commit.
⚡ Performance: ✅ LGTM
Visiting pending reads costs time linear in the number of pending reads, and it is behind the gc-complete-tracing autogate. That cost is acceptable.
📝 Documentation: ✅ LGTM
Fixed: src/workerd/jsg/README.md:260 and src/workerd/api/streams/README.md:470 now say that TracedOwn makes its handles strong on move only with the gc-complete-tracing autogate enabled.
Reviewed commit: 612c95ac
There was a problem hiding this comment.
Superseded by the review of
4ce968ae.
This review, of 1d4f90eb
✅ Approved — None of the reviewed files changed since the last approved review. The new commit is a merge from main, and a re-check of the core TracedOwn and stream-queue logic found no new issues.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
This change is about GC and handle lifetimes. It adds no input handling, authentication, secrets or injection surface.
📖 AGENTS.md: ✅ LGTM
build/AGENTS.md lists the jsg-traced-own check and its test target. src/workerd/jsg/AGENTS.md documents the TracedOwn rule.
📋 Code Quality: ✅ LGTM
I re-checked TracedOwn and GcVisitor::makeStrong in jsg/jsg.h, the changes to visitRef and visit in jsg/wrappable.c++, and how the stream queue handles pending reads. I found no defects:
- Moving after a visit: wherever a pending read is moved out of the ring buffer or the extracted vector,
TracedOwn's move makes its handles strong. - Autogate off: with
gc-complete-tracingdisabled, nothing visits the pending reads andTracedOwnbehaves likekj::Own. - Ring-buffer growth: growing the buffer moves each element, which makes its handles strong until the next visit. This is the documented behaviour.
close()anderror(): both make pending reads strong before the queue stops visiting them.
⚡ Performance: ✅ LGTM
Visiting pending reads takes time linear in how many there are, and only happens with the gc-complete-tracing autogate enabled. That cost is acceptable.
📝 Documentation: ✅ LGTM
The READMEs say correctly that TracedOwn makes its handles strong on a move only when the gc-complete-tracing autogate is enabled.
Reviewed commit: 1d4f90eb
Another kj::Own from kj::addRef() could keep the object alive after the TracedOwn is destroyed, with traced handles that nothing visits.
There was a problem hiding this comment.
✅ Approved — The only change since the last approved review adds a compile-time check that rejects refcounted types in jsg::TracedOwn. It is correct, and none of the current users is affected.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🔒 Security: ✅ LGTM
This change is about GC and handle lifetimes. It adds no untrusted input, authentication or injection surface.
📖 AGENTS.md: ✅ LGTM
The AGENTS.md files are still accurate.
📋 Code Quality: ✅ LGTM
The refcounted check in TracedOwn runs only where T must already be complete, so forward-declared members still compile. None of the wrapped types derives from kj::Refcounted: EventHandler, CryptoKey::Impl, WebSocketAdapter, Pipe, the readables, ReadRequest and Queue::Consumer. The moves and the autogate-off path behave as in the last review.
⚡ Performance: ✅ LGTM
No performance concerns in the jsg.h change.
📝 Documentation: ✅ LGTM
No documentation issues in the changed code.
Reviewed commit: 4ce968ae
Moving a
jsg::Datamakes it strong again, but moving thekj::Own<T>of an object that holds handles moves only the pointer. After a visit those handles stay traced with nothing visiting them, and GC can free them while the object still uses them. So code either didn't visit them, which leaks, or worked around it by hand likedrain()(687b3a6). The stream queue never visited the resolvers of pending reads, so aReadableStreamwhose read never settles was never collected.In this patch:
jsg::TracedOwn<T>is akj::Ownthat makes its object's handles strong when it moves or is released after a visit, through a newGcVisitor::makeStrong(). Same rule asmake_strongin Rust JSG (Add jsg::Cell and jsg::RefCell for traced fields #7982).TracedOwnand visits them, behind a newgc-complete-tracingautogate. With it disabled nothing visits them andTracedOwnbehaves likekj::Own, so behavior is the same as on main.jsg-traced-own, flags visiting through akj::Ownand moving a container whose elements hold aTracedOwn. The sites it found (stream readables, pipe events, EventTarget listeners,CryptoKey, WebSocket) now hold aTracedOwn.1,000 streams with a never-settling read, after GC: 0 collected before, 999 after.
workerd serve, every request leaves one such stream behind (-c opt, macOS):No difference on pending-read, tee, or hello-world requests.