cloudflare / cloudflare/capnweb

onRpcBroken should return a Disposable so registrations can be undone

Open
#234 1 comment 0 reactions 0 assignees Claimed by @srikrsna View on GitHub
enhancement
Dominant language
TypeScript
Stars
4k
Forks
143
Avg merge
4d 6h
Merged PRs (30d)
7

Description

**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

```js
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`:

```ts
interface StubBase extends Disposable {
onRpcBroken(callback: (error: any) => void): Disposable;
}
```

```js
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:

```js
{
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:

```ts
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:

```js
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.

Contributor guide

Open the contributing guide

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.

Assessment

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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.