onRpcBroken should return a Disposable so registrations can be undone

Open
  • Is something wrong?
#234 1 comment 0 reactions 0 assignees View on GitHub

Maintainers usually reply within 2 days

Nobody has claimed this yet.

  • #248 by @ting-hong-shieh — closed without merging
  • #250 by @srikrsna — closed without merging

Assessment

Difficulty
4/5
Estimated time
3-5 days
Newbie friendliness
35/100
Issue type
Feature
Clarity
Clearly specified
Activity status
Stale
Tech stack
typescript
Domain
api, backend-api-design

Research direction

Start by reading RpcImportHook.onBroken() in src/rpc.ts and the abstract StubHook.onBroken at src/core.ts:315, then inspect the implementations including the RpcTarget hook at src/core.ts:1967. Review the existing ordering tests and linked pull request #250. Done means the hooks return idempotent Disposable handles, including deferred PromiseStubHook forwarding, while preserving current cleanup and callback behavior.

Written by the indexing model from the issue text.

Description

enhancement

Version: capnweb 0.10.0. Applies to RpcStub.onRpcBroken / RpcPromise.onRpcBroken.

Summary

onRpcBroken() returns void, so a registration can never be undone. Registering the same callback twice fires it twice, and there is no offRpcBroken. I would like it to return a Disposable handle that unregisters the callback.

Why this matters

Every other subscription-shaped API in the ecosystem hands back something you can use to stop listening: addEventListener/removeEventListener, AbortSignal, or a returned unsubscribe function. onRpcBroken is the odd one out, and the places you naturally want to call it are exactly the places where things get re-run.

The clearest case is a UI framework. A React effect that registers a handler and re-runs (a dependency changes, StrictMode double-invokes, a component remounts against a session that outlives it) has no way to clean up after itself, so handlers accumulate on a long-lived stub and each break fires all of them. The workaround is to hoist registration to the point where the session is created, which works but constrains where the code can live, and pushes people toward a mutable "current handler" variable that the permanent callback dispatches through.

Our own documentation currently has to warn readers about this rather than show them the fix:

onRpcBroken cannot be unregistered. It returns nothing, and registering twice on the same stub fires twice. Register it where the session is created rather than in an effect that might re-run.

Reproduction

const ret = main.onRpcBroken(() => {});
console.log(ret);                    // undefined

let fires = 0;
const cb = () => { fires++; };
main.onRpcBroken(cb);
main.onRpcBroken(cb);                // same function, registered twice
transport.breakNow(new Error('connection lost'));
// fires === 2

Deduplicating by function identity would not be a fix on its own, since two independent subscribers legitimately registering the same function should each be honoured. What is missing is a handle.

Proposed solution

Return a Disposable:

interface StubBase<T = unknown> extends Disposable {
  onRpcBroken(callback: (error: any) => void): Disposable;
}

const subscription = stub.onRpcBroken(handleBroken);
// ...later
subscription[Symbol.dispose]();

which makes the ergonomic form work directly, and matches how the rest of the library already asks you to think about lifetimes:

{
  using subscription = stub.onRpcBroken(handleBroken);
  await doWorkThatMightBreak();
}   // unregistered here

Notes on the shape:

  • Non-breaking. The method currently returns undefined, so existing call sites that ignore the result keep working unchanged.
  • Idempotent. Disposing twice, or disposing after the callback has already fired, should be a no-op rather than an error.
  • Disposing the stub should continue to release registrations as it does today; the handle is for the narrower case of ending one subscription early.
  • Returning a bare () => void unsubscribe function is the other obvious option, but Disposable composes with using and is the convention the library has already committed to for stubs and for the revoker objects people build on top of Symbol.dispose.

Implementation sketch

The bookkeeping this needs is largely present. RpcImportHook.onBroken() (src/rpc.ts) already pushes onto the session-level array and records the slot index:

let index = this.session.onBrokenCallbacks.length;
this.session.onBrokenCallbacks.push(callback);
if (!this.onBrokenRegistrations) this.onBrokenRegistrations = [];
this.onBrokenRegistrations.push(index);

and resolve() already deletes individual slots with delete this.session.onBrokenCallbacks[i], preserving the ordering that existing tests pin. A returned handle would close over the hook and that index and perform the same deletion.

The abstract StubHook.onBroken (src/core.ts:315) and its implementations would change return type. Two are trivial (ErrorStubHook fires immediately, so it can return an already-disposed no-op handle; the RpcTarget hook at src/core.ts:1967 is currently a no-op). PromiseStubHook.onBroken forwards to a resolution that may not exist yet, so its handle needs to unregister from whichever hook it eventually forwarded to.

Relationship to #210

#210 reports that registrations on a disposed import are retained for the session's lifetime and fire at teardown. That is a bug in the same bookkeeping this feature would build on, and its fix sketch (mirroring resolve()'s cleanup on the disposal path) is roughly the operation a disposable handle needs to perform on demand. It probably makes sense to do #210 first, or to do both together.

Alternative worth considering alongside

An AbortSignal option would compose well with code that already has one for teardown:

stub.onRpcBroken(handleBroken, { signal: controller.signal });

Worth noting for anyone who finds this issue by searching: this would be a purely local API, so it is unaffected by the fact that AbortSignal cannot currently be serialized and sent over the wire.

One reason to settle this soon

onRpcBroken does not exist in the Workers Runtime's native RPC yet, and the two systems are converging deliberately. Fixing the signature while Cap'n Web is the only implementation avoids having to change it later in two places, one of which would need a compatibility flag.

Dominant language
TypeScript
Stars
4k
Forks
151
Avg merge
16h 46m
Merged PRs (30d)
2

Getting set up

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from cloudflare/capnweb

All issues in cloudflare/capnweb

Similar issues

More TypeScript issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.