Accept an AbortSignal in onRpcBroken and cleanup registrations when stubs are disposed

[srikrsna]

Closes #210 and #234

Adds an optional second parameter to onRpcBroken, with an optional signal. Arguably it could just be the signal, since I don't see us adding any more options. But this leaves room if we do.

#234 proposes that onRpcBroken return a Disposable. That lines up with the rest of the API, but it implies to the caller they're holding something that needs to be disposed. I don't know of a lint rule that enforces that, but I wouldn't be surprised if one exists.

I went with a signal for a few reasons:

• It's already an accepted, well-known pattern for other event listener APIs.
• It's opt-in, so we only pay the cost when a signal is set.
• After onRpcBroken callbacks are retained for the session's lifetime after their import is disposed (and fire at teardown) #210, most callers probably won't need this at all, so always returning a disposable seemed unnecessary.
• It lets you cancel several callbacks at once. A disposable can do that too, but a signal is easier when other event listeners are in the mix.

This PR also removes callbacks when a stub is disposed.

[changeset-bot]

🦋 Changeset detected

Latest commit: f956cb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
capnweb	Minor

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

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/cloudflare/capnweb@250

commit: f956cb4

[teamchong]

/bonk review this

[ask-bonk]

Review: AbortSignal support in onRpcBroken + dispose cleanup

Reviewed the full diff, built the project, and ran the node test project (all 386 tests pass, including the 6 anySignal tests and 8 onRpcBroken tests). This is a clean, well-scoped change. The design rationale in the PR description (signal over Disposable) is sound and matches the ecosystem convention for event-listener cancellation.

What I like

• Correct "already aborted registers nothing" semantics across every hook that stores or synchronously invokes the callback (ErrorStubHook, PromiseStubHook, ImportTableEntry). The throwError case in the tests (an already-broken stub that normally reports synchronously) is a nice edge case to cover.
• Per-hook abort controller in RpcImportHook (not per-entry) is the right call — dups share the entry, so composing each hook's controller with the user signal via anySignal keeps disposal/cancellation independent per dup. The "honors independent signals on separate dups" test locks this in.
• resolve() migration handles a canceled-before-resolve callback correctly by skipping undefined slots, and the "honors the signal after the promise resolved" test covers migration-then-abort.
• anySignal fallback correctly propagates reason, fires once, and drops source listeners via { signal: controller.signal } so a long-lived source signal doesn't retain listeners. Good test coverage on both native and fallback paths.
• Docs (README + changeset) clearly call out the behavior change: disposing a stub now drops its callbacks, scoped per-stub.

Observations (non-blocking)

1. Stale listeners after resolve() migration. In ImportTableEntry.resolve(), when a registration is migrated onto the resolution, the deregister listener added in onBroken() is left attached to both this.abortController.signal and the (composite) options.signal. They're now stale — deregister would delete an already-deleted onBrokenCallbacks[index] slot, so it's harmless, and the entry's abortController firing on dispose() eventually removes them. Just noting it's a small amount of retained state until disposal; not a correctness issue.

2. anySignal composite listener retained on the entry controller. RpcImportHook.onBroken builds anySignal([this.abortController.signal, options.signal]). The composite registers a listener on the hook's abortController.signal; on hook dispose() that controller aborts, which fires the composite (aborting it) and — via { signal: controller.signal } — self-cleans. So this is handled correctly.

3. AbortController/AbortSignal availability. signal.ts guards only AbortSignal.any, assuming AbortController/AbortSignal exist. That's fine for the supported runtimes (browsers, workerd, Node 16+), just flagging the implicit baseline.

Testing note

The default vitest run requires workerd + Playwright browsers (not available in every CI/local env). The signal + onRpcBroken logic is plain JS and lives in the node project, which passes cleanly:

Test Files  29 passed (29)
Tests       386 passed (386)

LGTM. The observations above are minor and don't need to block merge.

[ask-bonk]

Review posted to PR #250: #250 (comment)

Summary

I reviewed the full diff, built the project, and ran the node test project (386 tests pass, including the 6 anySignal and 8 onRpcBroken tests).

Verdict: LGTM. The change is clean, well-scoped, and thoroughly tested.

Key findings:

• Correct "already-aborted registers nothing" semantics across all relevant hooks (ErrorStubHook, PromiseStubHook, ImportTableEntry), including the tricky synchronous-report-when-already-broken case.
• Per-hook abort controller in RpcImportHook (rather than per-entry) is the right choice for independent per-dup cancellation, since dups share the underlying entry.
• resolve() migration correctly skips already-canceled callbacks.
• anySignal fallback correctly propagates reason, fires once, and self-cleans listeners.
• Docs/changeset clearly flag the behavior change (disposal now drops per-stub callbacks).

Non-blocking observations I noted: a small amount of stale-but-harmless listener state left on the entry after resolve() migration until disposal, and the implicit AbortController/AbortSignal runtime baseline. Neither needs to block merge.

One environment note: the default vitest run requires workerd + Playwright browsers which aren't available here, but the signal logic is plain JS covered by the node project, which passes fully.

github run

[ask-bonk]

@srikrsna Bonk workflow was cancelled.

View workflow run · To retry, trigger Bonk again.

[ask-bonk]

No toolchain available, but the code analysis is complete.

Summary of review

The PR adds an optional AbortSignal to onRpcBroken and drops callbacks on stub disposal. I reviewed all 14 changed files in depth:

• signal.ts anySignal — correct: already-aborted short-circuit, single-fire, reason propagation, and self-cleaning listeners via { signal: controller.signal }.
• rpc.ts — the per-hook abortController (vs. per-entry) is the right choice for independent per-dup cancellation since dups share the entry. The resolve() migration correctly skips canceled slots and preserves registration ordering. The already-aborted-registers-nothing semantics are honored in ErrorStubHook, PromiseStubHook, and ImportTableEntry.
• core.ts / streams.ts / map.ts — signature threading is consistent; the synchronous-report-when-already-broken case in ErrorStubHook and PromiseStubHook correctly checks signal.aborted.
• Type/API surface (types.d.ts, index.ts, capnweb-validate) — OnRpcBrokenOptions is exported and mirrored in the validate package. Purely additive, no breaking changes.
• Changeset + README — present and accurately describe the behavior change (disposal now drops per-stub callbacks).

The only observations are non-blocking and already noted in the existing thread (stale-but-harmless deregister listeners retained on the entry after migration until disposal). No logic bugs, security issues, capability leaks, wire-protocol changes, or backward-compat violations found. The change is well-scoped and thoroughly tested.

I was unable to run the test suite (dependencies not installed in this environment), but the signal/onRpcBroken logic is plain JS and the reasoning holds.

LGTM

github run

[teamchong]

Nit: outside the scope of this PR, but noticed we had Promise.resolve(err) with comment // Treat this as an unhandled rejection. Should this be Promise.reject(err) like in disposeRpcTarget (line 1877)?

[kentonv]

I think I prefer the disposable:

• It's much cheaper than allocating an AbortSignal. AbortSignal and AbortController are DOM APIs probably backed by native objects (in workerd, they are implemented as C++ objects). AbortSignal is an EventSource that can have many listeners registered. Signalling through them is complex machinery. A disposable, meanwhile, is a simple JavaScript object with one method.
• Cap'n Web heavily uses disposables already. It's weird to make people deal with both patterns at the same time. I'm think of React effects and such, it'd be weird to be creating a stub and arranging to dispose it, but also arranging an onRpcBroken() callback on it, and that has an AbortController -- bleh.
• A linter that lints leaked disposables will already be problematic with Cap'n Web. Consider an RpcPromise -- it is disposable, but disposing it is equivalent to disposing the eventual result. You only need to dispose one or the other, not both. Some apps will dispose the promise, some its result. A linter rule would presumably insist on disposing both.
• You kind of do have to dispose the onRpcBroken disposer, unleass you intend for it to stay live until the connection is lost. It turns out the same rule applies to all other disposables in Cap'n Web. You don't need to dispose a stub if the connection is lost -- it is disposed implicitly. So it seems like there isn't much difference here.

So I'd prefer the disposable API over the AbortSignal API.

However, there may be one other alternative to explore: What if we just don't add any new API here at all, and tell people to dispose the stub when they want the callback to be unregistered? If someone wants the ability to unregister a callback without disposing the stub, they can simply dup() the stub and register the callback on that -- and then dispose the dup to unregister the callback.