Repository navigation
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
Hi @jasnell — this PR addresses the retention reported in #7202, and it builds directly on the capture model you introduced in What changed: the five plain lambda captures in Tests: |
fb3e425 to
b632c99
Compare
|
Hi @jasnell — I've rebased onto current |
b632c99 to
ddba9f4
Compare
ddba9f4 to
0a03187
Compare
…n be collected Socket::close() chains several jsg::Promise continuations that capture the Socket via JSG_THIS (and one that captures the writable abort promise). Since 67f730b these were plain lambda captures, i.e. untraced jsg::Refs, which act as strong GC roots. When close() is called without being awaited while buffered writes keep the internal flush pending past IoContext teardown, the resulting cycle (Socket -> writable -> flush promise -> continuation -> Socket) can never be collected. The Socket, its promises and every AsyncContextFrame reachable from them (AsyncLocalStorage stores of finished requests) stay alive for the lifetime of the isolate. This is the retention reported in cloudflare#7202. Wrap the five captures in JSG_VISITABLE_LAMBDA so GcVisitor can trace them. The captures still keep the Socket alive for as long as a continuation is reachable (from the pending flush's resolver or the microtask queue), which is what 67f730b needed; only the unreachable case becomes collectable. Add socket-close-als-gc-test (legacy and TypeScript streams variants) which fails on the previous code with 12/12 retained stores for both a directly connected socket and a socket transferred over RPC, and passes with the fix. The passive control (no close()) shows that pending promises alone are collected after teardown. IoContext cancellation semantics are unchanged: whether promises settle at teardown is tracked separately. Fixes cloudflare#7202
0a03187 to
260ef57
Compare
|
Hi @guybedford — following up with someone who has direct context on this area: you authored #7313 which changed the same The fix addresses the ALS frame retention reported in #7202: plain lambda captures act as unconditional GC roots, keeping the If you have a moment, a review would be very welcome — and as an external contributor I'd also appreciate your help triggering the |
Fixes #7202
Problem
Socket::close()chains severaljsg::Promisecontinuations that capture theSocketviaJSG_THIS(plus one that captures the writable abort promise). Since 67f730b these have been plain lambda captures. Ajsg::Refthat theGcVisitornever sees stays a strongv8::Global, i.e. a GC root.When
close()is called without being awaited while buffered writes keep the internal flush pending pastIoContextteardown, the cycleis rooted from inside itself and can never be collected. The
Socket, itsclosed/openedpromises and everyAsyncContextFramereachable from them (theAsyncLocalStoragestores of long-finished requests) stay alive for the lifetime of the isolate. This is the retention reported in #7202.While investigating I checked the issue's own hypothesis (unsettled promises at teardown) and could not reproduce it: with
--expose-gcandWeakRefs,closed.then(...)/reader.read().then(...)registered insideals.run()on a socket that is never closed are collected normally once the request context is gone. The leak needs the pendingclose().Fix
Wrap the five captures in
JSG_VISITABLE_LAMBDAso the visitor traces them. The captures still keep theSocketalive while a continuation is reachable (from the pending flush's resolver held by KJ, or from the microtask queue), which is what 67f730b needed to fix its use-after-free; only the unreachable case becomes collectable.socket-close-gc-test(the UAF regression test) still passes, including the@gc-stressvariant.No change to
IoContextcancellation semantics. Whether pending promises should be settled at teardown is a separate question and is left out of this PR.Tests
socket-close-als-gc-test(legacy streams) andsocket-close-als-gc-ts-test(TypeScript streams), three cases each:pendingCloseCollectsclose()without awaiting, returnrpcTransferredPendingCloseCollectspendingWithoutCloseCollectsclose()A fixture guard asserts that
close()was still pending when each request returned, measured inside the request, so the test does not depend on what happens to the promise after teardown.Verification
bazel build //src/workerd/...clean.bazel teston the new tests plussocket-close-gc-test,js-rpc-socket-test,js-rpc-socket-streams-ts-test,connect-handler-test: all default,@all-compat-flags,@all-autogates,@eslintand@gc-stressvariants pass.main'ssockets.c++to confirm the new tests fail without the fix (output above).IoContextteardown both before and after: the retention was JS-heap only, no fd leak.tools/cross/format.py --checkclean.