Repository navigation
Conversation
Final disposal of an unresolved import now deletes its registrations from
the session's callback list, so they are no longer retained for the
session's lifetime and called at teardown.
Registrations become {callback, index} records shared by the session and
the owning import, so a callback moved by a same-session resolve() keeps
its original slot and stays removable by its new owner. Session abort
detaches the list before running app code, so a callback that disposes
another stub cannot unregister callbacks that have yet to fire.
Closes #210
🦋 Changeset detectedLatest commit: 3655f16 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
|
LGTM |
|
This pull request's reviews are posted as pull request reviews: see the review of |
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://fix-on-rpc-broken-dispose.pr.capnweb.com (commit 3655f16)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
There was a problem hiding this comment.
I think the following behavior is worth documenting with a test:
it("callbacks are retained by dups", async () => {
let harness = new TestHarness(new TestTarget());
let counter = await harness.stub.makeCounter(0);
let calls = 0;
counter.onRpcBroken(() => { ++calls; });
let dup = counter.dup();
counter[Symbol.dispose]();
harness.clientTransport.forceReceiveError(new Error("test disconnect"));
await pumpMicrotasks();
expect(calls).toBe(1);
});| // here. So, let's move the newly-added registration into the original's slot, updating | ||
| // its index so that its new owner can still unregister it. | ||
| // TODO: This is quite hacky, think about whether this is really the right answer. | ||
| moved.index = index; | ||
| this.session.onBrokenCallbacks[index] = moved; |
There was a problem hiding this comment.
Just a note, nothing to change but I think this has become even more hacky now as it depends on the newly created registration (the index update) in a different import hook along side the session.
There was a problem hiding this comment.
Agreed, it now writes into the new owner's record on top of the existing endIndex check. I left it since it's the same TODO, and this handoff is what #234 has to revisit anyway: a handle created before a promise resolves needs to keep working after its callback moves. That seems like the right place to decide what this should look like.
![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
3655f163.
This review, of 25731264
💬 Approved with comments — The fix is correct and tested, and the new test covers dup retention as requested in the discussion. One INFO note remains about stale registration indexes during abort. It can't make any callback fire wrongly.
Findings: 1 info
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
📋 Code Quality: 💬 1 info
The fix is correct. The new dup test covers the retention behaviour requested in the discussion. One informational note about the abort window remains.
Recommendation: 💬 Optional Nits
- [INFO] During abort, imports keep registration indexes that point into the detached callback array, while
dispose()andresolve()write to the fresh one. A stale index could delete anonRpcBrokenregistered during the abort window. No callback fires wrongly, because the fresh array is never iterated. (src/rpc.ts:272)
View full review
Stale indexes during abort (INFO, unchanged since the last review)
src/rpc.ts:940-941:abort()swapsonBrokenCallbacksfor a fresh[]before any app code runs.- Imports keep their
onBrokenRegistrationsuntil the final abort loop, so theirindexvalues still point into the detached array. - If a stub is disposed in that window,
dispose()atsrc/rpc.ts:272deletes an entry from the fresh array. If app code registered a newonRpcBrokenin the same window, that delete could remove the unrelated registration.resolve()atsrc/rpc.ts:240-244can make the same kind of write. - The fresh array is never iterated after an abort, so no callback ever fires wrongly.
- The description says registration during abort was left alone on purpose.
- A cheap guard, if you want one: skip the delete when
session.abortReason !== undefined.
Tests
The new test callbacks are retained by dups (__tests__/index.test.ts:2434) pins down the retention behaviour suggested in the discussion. Unregistering on full disposal is already covered by does not call back for capabilities disposed before the connection is lost.
resolve() handoff
The discussion on the resolve() handoff (src/rpc.ts:240) is noted. Writing into the new owner's record there is an existing TODO, and #234 is the natural place to rework it.
✅ All other checks passed
🚀 Release: ✅ LGTM
The changeset is present and appropriate for a bug fix. Risk: low.
🔒 Security: ✅ LGTM
No security concerns in this change.
📖 AGENTS.md: ✅ LGTM
No agent instruction files are affected.
⚡ Performance: ✅ LGTM
No performance concerns. Disposal and abort do O(1) work per registration, and disposed stubs no longer keep their callbacks alive for the lifetime of the session.
📝 Documentation: ✅ LGTM
The disposal and sessions guides accurately describe the new behaviour: callbacks are unregistered when a stub is disposed.
Reviewed commit: 25731264
There was a problem hiding this comment.
✅ Approved — The fix is correct and well tested. The new commits add tests for dup retention and for parameter stubs, plus accurate docs, and they introduce no issues. One earlier informational note about stale indexes during abort remains open as the author's choice.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
The changeset is present and appropriate for a bug fix. Risk: low.
🔒 Security: ✅ LGTM
No security concerns in this change.
📖 AGENTS.md: ✅ LGTM
No agent instruction files are affected.
📋 Code Quality: ✅ LGTM
Since the last review, this pull request adds tests and updates the docs. I found no problems in them.
__tests__/index.test.tscovers four behaviours, and all four match the code insrc/rpc.ts:- callbacks are unregistered once the stub is fully disposed, including a callback that
resolve()moved from a promise to a stub; - callbacks are kept while a
dupis alive; - a callback still fires when an earlier callback has already disposed its stub, because
abort()detaches the callback list first; - a callback on a parameter stub fires only if a
dupoutlives the call.
- callbacks are unregistered once the stub is fully disposed, including a callback that
disposal.mdx,stubs.mdxandsessions.mdxdescribe what the code does.
One earlier note, about stale registration indexes during abort, is still open. Its code has not changed and nobody has replied to it, so it is left to the author.
⚡ Performance: ✅ LGTM
No performance concerns.
- Disposal,
resolve()and abort each do O(1) work per registration. - Disposed stubs no longer keep their callbacks alive for the life of the session.
- Deleting entries leaves holes in the sparse callback array. Those holes cost far less memory than the callbacks that used to be retained.
📝 Documentation: ✅ LGTM
The changeset and the disposal, stubs and sessions docs accurately describe the new behaviour. This includes the note about parameter stubs and the advice to register onRpcBroken() on the stub you keep.
Reviewed commit: 3655f163
Closes #210
fixes a leak on long-lived connections. A server or client that keeps making and dropping capabilities, and watches each one with onRpcBroken(), used to keep every callback it ever registered. When the connection finally dropped, all of them ran, including callbacks for objects the app had cleaned up already. Disposing a remote stub other than the session's main stub, along with all its duplicates, now unregisters the
onRpcBroken()callbacks registered on it. (Disposing the main stub still ends the session, which calls them.)Deleting the slots on disposal, as the issue sketches, only covers callbacks registered directly on a stub. A callback registered on a promise that resolves to a stub on the same session gets moved by
resolve(), which keeps the original slot so callbacks still fire in registration order, while the new owner records the index of a slot that was just deleted. Registrations are now{callback, index}records shared by the session and the owning import, soresolve()can hand the original slot to the new owner along with its index.#234 asks for a way to unregister a single callback. This PR doesn't add one: tracking stays per import, so callbacks go away only once the stub and every duplicate are disposed, and disposing just one dup unregisters nothing. #234 may reshape the records and the
resolve()handoff, but the behavior and tests here hold under either design proposed there. Draft #284 targets the same issue; it also changes registration during abort andresolve()reentrancy, which are separate pre-existing behaviors I left alone.I went with unregistering on disposal, rather than calling the callbacks at that point, for a few reasons:
AbortSignalinonRpcBrokenand cleanup registrations when stubs are disposed #250: dispose the stub to unregister its callback.Ref.whenBrokenfollows the same rule: a reference that is collected before a partition doesn't need to inform its reactor.This PR also detaches the session's callback list at the start of an abort, before any app code runs. Without that, a broken-connection handler that disposes child stubs would unregister the children's callbacks before they fire. As a side effect, the session no longer holds any callbacks after it ends.
One change users may notice: a stub received as a call parameter is disposed when the call returns, so an
onRpcBroken()callback on it no longer fires at disconnect unless the stub wasdup()ed. The changeset and the disposal and stubs docs say so, and a test covers it. The docs also advise callingonRpcBroken()on the stub you keep, because adup()of a promise that hasn't been awaited is a separate import and does not keep the promise's callbacks. The sessions guide no longer saysonRpcBroken"cannot be unregistered".