onRpcBroken should return a Disposable so registrations can be undone

Đang mở
  • Có gì không ổn?
#234 1 bình luận 0 reaction 0 người được giao Xem trên GitHub

Maintainer thường phản hồi trong vòng 5 ngày

Chưa có ai nhận issue này.

  • #248 của @ting-hong-shieh — đã đóng, không merge
  • #250 của @srikrsna — đã đóng, không merge

Đánh giá

Độ khó
4/5
Thời gian dự kiến
3-5 ngày
Mức phù hợp với người mới
35/100
Loại issue
Tính năng
Độ rõ ràng
Đặc tả rõ ràng
Mức độ hoạt động
Đình trệ
Công nghệ
typescript
Lĩnh vực
api, backend-api-design

Hướng nghiên cứu

Bắt đầu bằng cách đọc RpcImportHook.onBroken() trong src/rpc.ts và StubHook.onBroken trừu tượng tại src/core.ts:315, sau đó kiểm tra các triển khai, bao gồm hook RpcTarget tại src/core.ts:1967. Xem lại các bài kiểm tra ordering hiện có và pull request được liên kết #250. Công việc được coi là hoàn tất khi các hook trả về các handle Disposable có tính idempotent, bao gồm việc chuyển tiếp PromiseStubHook bị trì hoãn, đồng thời duy trì hành vi cleanup và callback hiện tại.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Mô tả

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.

Ngôn ngữ chính
TypeScript
Star
4k
Fork
150
Merge trung bình
3 ngày 12 giờ
Pull request đã merge (30 ngày)
2

Chuẩn bị môi trường

Bắt đầu từ đâu

  1. Đọc hết issue, rồi đọc hướng dẫn đóng góp của dự án.
  2. Bình luận trên issue rằng bạn sẽ nhận — tránh hai người làm cùng một việc.
  3. Fork repository và làm thay đổi trên một nhánh.
  4. Mở pull request có tham chiếu số hiệu của issue.

Issue khác của cloudflare/capnweb

Tất cả issue của cloudflare/capnweb

Issue tương tự

Thêm issue về TypeScript

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.