diff --git a/.changeset/promise-stubhook-disposal.md b/.changeset/promise-stubhook-disposal.md new file mode 100644 index 0000000..26d72b2 --- /dev/null +++ b/.changeset/promise-stubhook-disposal.md @@ -0,0 +1,5 @@ +--- +"capnweb": patch +--- + +Fix RPC argument and capture leaks on failure paths — rejected or broken destination hooks and local call errors now dispose the arguments they own — and keep `PromiseStubHook` disposal ordered behind already-queued calls. diff --git a/.changeset/rpc-promise-from-promise.md b/.changeset/rpc-promise-from-promise.md new file mode 100644 index 0000000..9f9e7d5 --- /dev/null +++ b/.changeset/rpc-promise-from-promise.md @@ -0,0 +1,5 @@ +--- +"capnweb": minor +--- + +`RpcPromise` can now be constructed by the application from a `Promise` for the eventual resolution. Calls pipeline immediately and are queued, in order, until the promise settles, making it possible to publish a capability that doesn't exist yet, e.g. while re-establishing a broken session. diff --git a/README.md b/README.md index f73d017..a1dc73e 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,28 @@ let profile = await api.getUserProfile(user.id); Whenever an `RpcPromise` is passed in the parameters to an RPC, or returned as part of the result, the promise will be replaced with its resolution before delivery to the receiving application. So, you can use an `RpcPromise` anywhere where a `T` is required! +#### Constructing `RpcPromise` from a `Promise` + +You can construct an `RpcPromise` directly from a regular `Promise`, allowing you to perform promise pipelining on a regular local promise. Pipelined calls will wait until the inner promise resolves, then will be delivered, in-order, to the resolution. This is useful when you plan to obtain some stub in the future, but you want to allow code to start queuing calls on it immediately. + +Wrapping a `Promise` in this way is semantically identical to creating a local-loopback RPC and then invoking it. That is: + +```ts +// this... +let rpcPromise = new RpcPromise(myPromise); + +// is semantically the same as this... +let rpcFunc = new RpcStub(() => myPromise); +let rpcPromise = rpcFunc(); +``` + +In other words, this means: +* The result of the promise must be serializable. +* If the promise resolution contains `RpcTarget`s or `Function`s, the `RpcPromise`'s resolution will replace them with stubs. +* Ownership of any stubs in the Promise result is transferred away. If you want to keep your own copies, you need to `dup()` them. +* If the promise rejects, the rejection propagates to all pipelined calls. +* etc. + ### The magic `map()` method Every RPC promise has a special method `.map()` which can be used to remotely transform a value, without pulling it back locally. Here's an example: diff --git a/__tests__/index.test.ts b/__tests__/index.test.ts index 00adc71..5123705 100644 --- a/__tests__/index.test.ts +++ b/__tests__/index.test.ts @@ -4,11 +4,13 @@ import { expect, it, describe, inject } from "vitest" import { deserialize, serialize, RpcSession, type RpcSessionOptions, RpcTransport, - type RpcTransportWithCustomEncoding, RpcTarget, RpcStub, newWebSocketRpcSession, + type RpcTransportWithCustomEncoding, RpcTarget, RpcStub, RpcPromise, newWebSocketRpcSession, newMessagePortRpcSession, newHttpBatchRpcSession} from "../src/index.js" import { swapByteOrder } from "../src/serialize.js" import { MAX_CLOSE_REASON_BYTES } from "../src/websocket.js" +import { ErrorStubHook, PayloadStubHook, PromiseStubHook, RpcPayload, RpcStub as RawRpcStub, + unwrapStubTakingOwnership } from "../src/core.js" import { Counter, TestTarget } from "./test-util.js"; type CustomEncodingLevel = RpcTransportWithCustomEncoding["encodingLevel"]; @@ -547,6 +549,7 @@ class TestTransport implements RpcTransport { private waiter?: () => void; private aborter?: (err: any) => void; public log = false; + public sentLog: string[] = []; private fenced = false; send(message: string): void { @@ -555,6 +558,7 @@ class TestTransport implements RpcTransport { message = message.replaceAll("$remove$", ""); if (this.log) console.log(`${this.name}: ${message}`); + this.sentLog.push(message); this.partner!.queue.push(message); if (this.partner!.waiter && !this.partner!.fenced) { this.partner!.waiter(); @@ -2178,6 +2182,289 @@ describe("onRpcBroken", () => { // ======================================================================================= +describe("PromiseStubHook", () => { + it("disposes copied call arguments when the backing promise rejects", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let argument = new RpcStub(new Disposable()); + let hook = new PromiseStubHook(Promise.reject(new Error("nope"))); + let result = hook.call([], RpcPayload.fromAppParams([argument])); + + await expect(result.pull()).rejects.toThrow("nope"); + argument[Symbol.dispose](); + expect(disposed).toBe(true); + }); + + it("disposes copied stream arguments when the backing promise rejects", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let argument = new RpcStub(new Disposable()); + let hook = new PromiseStubHook(Promise.reject(new Error("nope"))); + let result = hook.stream(["write"], RpcPayload.fromAppParams([argument])); + + await expect(result.promise).rejects.toThrow("nope"); + argument[Symbol.dispose](); + expect(disposed).toBe(true); + }); + + it("delivers a call initiated before disposal", async () => { + let disposed = false; + class DisposableCounter extends Counter { + [Symbol.dispose]() { disposed = true; } + } + + let inner = new RpcStub(new DisposableCounter(1)); + let hook = new PromiseStubHook(Promise.resolve(unwrapStubTakingOwnership(inner))); + await pumpMicrotasks(); + + let stub: RpcStub = new RawRpcStub(hook); + let result = stub.increment(2); + stub[Symbol.dispose](); + + expect(disposed).toBe(false); + expect(await result).toBe(3); + expect(disposed).toBe(true); + }); + + it("disposes copied call arguments when the destination hook is broken", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let argument = new RpcStub(new Disposable()); + let hook = new PromiseStubHook(Promise.resolve(new ErrorStubHook(new Error("broken")))); + let result = hook.call([], RpcPayload.fromAppParams([argument])); + + await expect(result.pull()).rejects.toThrow("broken"); + argument[Symbol.dispose](); + expect(disposed).toBe(true); + }); + + it("disposes copied stream arguments when the destination hook is broken", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let argument = new RpcStub(new Disposable()); + let hook = new PromiseStubHook(Promise.resolve(new ErrorStubHook(new Error("broken")))); + let result = hook.stream(["write"], RpcPayload.fromAppParams([argument])); + + await expect(result.promise).rejects.toThrow("broken"); + argument[Symbol.dispose](); + expect(disposed).toBe(true); + }); + + it("disposes copied call arguments when the local call path fails", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let argument = new RpcStub(new Disposable()); + let hook = new PromiseStubHook( + Promise.resolve(new PayloadStubHook(RpcPayload.fromAppReturn({})))); + let result = hook.call(["nope"], RpcPayload.fromAppParams([argument])); + + await expect(result.pull()).rejects.toThrow("'nope' is not a function"); + argument[Symbol.dispose](); + expect(disposed).toBe(true); + hook.dispose(); + }); + + it("disposes map captures when the destination hook is broken", () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let capture = unwrapStubTakingOwnership(new RpcStub(new Disposable())); + let hook = new ErrorStubHook(new Error("broken")); + hook.map([], [capture], []); + expect(disposed).toBe(true); + }); +}); + +// ======================================================================================= + +describe("constructing RpcPromise from a promise", () => { + it("pipelines through a pending promise without pulling the resolution", async () => { + await using harness = new TestHarness(new TestTarget()); + + let {promise, resolve} = Promise.withResolvers>(); + using stub = new RpcPromise(promise); + + using counter = stub.makeCounter(1); + let result = counter.increment(2); + resolve(harness.stub.dup()); + expect(await result).toBe(3); + + // Only the final result was pulled: neither the promise's resolution nor the intermediate + // counter was transmitted. + let sent = harness.clientTransport.sentLog; + expect(sent.some(msg => msg.startsWith('["push"'))).toBe(true); + expect(sent.filter(msg => msg.startsWith('["pull"'))).toHaveLength(1); + }); + + it("queues calls made before resolution and delivers them in order", async () => { + let calls: number[] = []; + class Recorder extends RpcTarget { + record(i: number) { calls.push(i); return i; } + } + + let {promise, resolve} = Promise.withResolvers(); + using stub = new RpcPromise(promise); + + let results = [stub.record(1), stub.record(2), stub.record(3)]; + expect(calls).toStrictEqual([]); + + resolve(new Recorder()); + expect(await Promise.all(results)).toStrictEqual([1, 2, 3]); + expect(calls).toStrictEqual([1, 2, 3]); + }); + + it("accepts a promise for a target, a remote stub, or a plain value", async () => { + await using harness = new TestHarness(new TestTarget()); + + using target = new RpcPromise(Promise.resolve(new Counter(1))); + expect(await target.increment()).toBe(2); + + using remote = new RpcPromise(Promise.resolve(harness.stub.dup())); + expect(await remote.square(3)).toBe(9); + + using value = new RpcPromise<{foo: number}>(Promise.resolve({foo: 123})); + expect(await value.foo).toBe(123); + }); + + it("awaiting the RpcPromise yields the resolution", async () => { + using plain = new RpcPromise<{foo: number}>(Promise.resolve({foo: 123})); + expect(await plain).toStrictEqual({foo: 123}); + + await using harness = new TestHarness(new TestTarget()); + using remote = new RpcPromise(Promise.resolve(harness.stub.dup())); + let resolved = await remote; + expect(await resolved.square(4)).toBe(16); + }); + + it("reports rejection to queued calls, await, and onRpcBroken", async () => { + let error = new Error("nope"); + using stub = new RpcPromise(Promise.reject(error)); + + let broken: any[] = []; + stub.onRpcBroken(err => { broken.push(err); }); + + await expect(() => stub.increment()).rejects.toThrow("nope"); + await expect(Promise.resolve(stub)).rejects.toThrow("nope"); + expect(broken).toStrictEqual([error]); + }); + + it("does not report an unhandled rejection for an unused promise", async () => { + new RpcPromise(Promise.reject(new Error("ignored"))); + await pumpMicrotasks(); + }); + + it("does not report an unhandled rejection for a discarded queued call", async () => { + using stub = new RpcPromise(Promise.reject(new Error("ignored"))); + stub.increment(); // result intentionally discarded + await pumpMicrotasks(); + }); + + it("does not report an unhandled rejection for a discarded map() result", async () => { + using stub = new RpcPromise(Promise.reject(new Error("ignored"))); + stub.map(i => i); // result intentionally discarded + await pumpMicrotasks(); + }); + + it("disposes the eventual target when disposed before resolution", async () => { + let disposed = false; + class Disposable extends RpcTarget { + [Symbol.dispose]() { disposed = true; } + } + + let {promise, resolve} = Promise.withResolvers(); + let stub = new RpcPromise(promise); + stub[Symbol.dispose](); + + resolve(new Disposable()); + await pumpMicrotasks(); + expect(disposed).toBe(true); + }); + + it("delivers a call initiated before disposal", async () => { + let disposed = false; + class DisposableCounter extends Counter { + [Symbol.dispose]() { disposed = true; } + } + + let stub = new RpcPromise(Promise.resolve(new DisposableCounter(1))); + await pumpMicrotasks(); + + let result = stub.increment(2); + stub[Symbol.dispose](); + + expect(disposed).toBe(false); + expect(await result).toBe(3); + expect(disposed).toBe(true); + }); + + it("keeps an adopted RpcPromise lazy", async () => { + await using harness = new TestHarness(new TestTarget()); + + using counter = new RpcPromise(harness.stub.makeCounter(1)); + expect(await counter.increment(2)).toBe(3); + + let sent = harness.clientTransport.sentLog; + expect(sent.filter(msg => msg.startsWith('["pull"'))).toHaveLength(1); + }); + + it("preserves brokenness of a bare stub it was constructed from", async () => { + await using harness = new TestHarness(new TestTarget()); + using stub = new RpcPromise(harness.stub.dup()); + + let errors: any[] = []; + stub.onRpcBroken(error => { errors.push(error); }); + + harness.clientTransport.forceReceiveError(new Error("test disconnect")); + await pumpMicrotasks(); + expect(errors).toStrictEqual([new Error("test disconnect")]); + }); + + it("keeps disposal idempotent when constructed from a bare stub", async () => { + let disposals = 0; + class Disposable extends RpcTarget { + [Symbol.dispose]() { ++disposals; } + } + + let inner = new RpcStub(new Disposable()); + let outer = new RpcPromise(inner); + inner[Symbol.dispose](); + outer[Symbol.dispose](); + + await pumpMicrotasks(); + expect(disposals).toBe(1); + }); + + it("resolves when awaited after construction from a bare local stub", async () => { + // Regression test: the constructor previously adopted a bare stub's hook directly, producing + // a promise whose pipelined calls worked but whose await rejected, because non-promise hooks + // don't implement pull(). + using stub = new RpcPromise(new RpcStub(new Counter(1))); + + expect(await stub.increment(2)).toBe(3); + let resolved = await stub; + expect(await resolved.increment(3)).toBe(6); + }); +}); + +// ======================================================================================= + describe("HTTP requests", () => { it("can perform a batch HTTP request", async () => { let cap = newHttpBatchRpcSession(`http://${inject("testServerHost")}`); diff --git a/__type-tests__/rpc-base-cases.test.ts b/__type-tests__/rpc-base-cases.test.ts index 1850dd3..262ec60 100644 --- a/__type-tests__/rpc-base-cases.test.ts +++ b/__type-tests__/rpc-base-cases.test.ts @@ -150,3 +150,29 @@ api.invoke((name: string, attempt: number) => { // @ts-expect-error headers argument must be Headers api.roundTripHeaders(new Map([["x-id", "1"]])) + +// An RpcPromise can be constructed from a promise for a plain value, an RpcTarget, or a stub, +// keeping the resolution's type in each case. +expectType>(new RpcPromise(Promise.resolve(42))) +expectType>(new RpcPromise(Promise.resolve(new PointTarget()))) +expectType>(new RpcPromise(Promise.reject(new Error("x")))) + +// The target type is inferred exactly from a promise for a stub -- no explicit type argument. +const promisedFromStub = new RpcPromise(Promise.resolve(pointStub)) +type _PromisedFromStubInfersTarget = Expect>> + +async function assertAwaitedConstructedPromiseShapes() { + const target = await new RpcPromise(Promise.resolve(new PointTarget())) + expectType>(target) + + const value = await new RpcPromise(Promise.resolve(42)) + expectType(value) +} + +void assertAwaitedConstructedPromiseShapes + +// @ts-expect-error a non-thenable value cannot back an RpcPromise +void new RpcPromise(42) + +// @ts-expect-error a bare stub cannot back an RpcPromise; pass a promise for the stub instead +void new RpcPromise(pointStub) diff --git a/src/core.ts b/src/core.ts index 86d8c03..99e1cdd 100644 --- a/src/core.ts +++ b/src/core.ts @@ -318,8 +318,15 @@ export abstract class StubHook { export class ErrorStubHook extends StubHook { constructor(private error: any) { super(); } - call(path: PropertyPath, args: RpcPayload): StubHook { return this; } - map(path: PropertyPath, captures: StubHook[], instructions: unknown[]): StubHook { return this; } + // call() and map() take ownership of `args` / `captures`; there is no callee here to consume + // them, so dispose them before reporting the error. + call(path: PropertyPath, args: RpcPayload): StubHook { args.dispose(); return this; } + map(path: PropertyPath, captures: StubHook[], instructions: unknown[]): StubHook { + for (let cap of captures) { + cap.dispose(); + } + return this; + } get(path: PropertyPath): StubHook { return this; } dup(): StubHook { return this; } pull(): RpcPayload | Promise { return Promise.reject(this.error); } @@ -534,9 +541,48 @@ export class RpcStub extends RpcTarget { } export class RpcPromise extends RpcStub { - // TODO: Support passing target value or promise to constructor. - constructor(hook: StubHook, pathIfPromise: PropertyPath) { - super(hook, pathIfPromise); + // Internally, an `RpcPromise` is constructed from a `StubHook` plus a property path. The + // application may instead pass a promise for the eventual resolution; calls made before it + // settles are queued and delivered, in order, once it does. + constructor(hook: StubHook | PromiseLike, pathIfPromise?: PropertyPath) { + if (hook instanceof StubHook) { + super(hook, pathIfPromise!); + } else { + if (pathIfPromise !== undefined) { + throw new TypeError("RpcPromise constructor expected one argument, received two."); + } + + if (typeForRpc(hook) === "rpc-promise") { + // Adopt an existing `RpcPromise` directly, transferring ownership of its hook. In + // particular, this keeps the adopted promise lazy -- assimilating it as a thenable would + // instead force its resolution to be pulled -- and preserves hook-local behavior such as + // brokenness. This applies only to promises, not bare stubs: a non-promise hook may not + // implement pull(), so a bare stub takes the generic path below, which adopts the stub + // into the resolution payload. + super(unwrapStubTakingOwnership(hook), []); + } else { + // `Promise.resolve()` natively handles the hazards of assimilating an arbitrary thenable + // (`then` getters with side effects, self-resolution, cross-realm thenables), so the + // resolution callback below only ever sees settled, non-thenable values. + // + // The resolution is adopted with "return" semantics, taking ownership of any stubs + // within (including a stub as the root value). This is the same representation used for + // the resolution of a local async call: pull() delivers the value, pipelined calls + // forward through the payload without forcing a pull, and a single-stub payload forwards + // onBroken(), preserving brokenness. + // + // A rejection is adopted as an ErrorStubHook rather than left to reject the backing + // promise. This way the promise chains backing queued calls never reject -- the calls + // land on the ErrorStubHook, which disposes their arguments -- and the error only + // surfaces through pull(), so a discarded pipelined call can't produce an unhandled + // rejection event. + let promiseHook = new PromiseStubHook(Promise.resolve(hook).then( + value => new PayloadStubHook(RpcPayload.fromAppReturn(value)), + err => new ErrorStubHook(err))); + promiseHook.ignoreUnhandledRejections(); + super(promiseHook, []); + } + } } then(onfulfilled?: ((value: unknown) => unknown) | undefined | null, @@ -1717,6 +1763,8 @@ abstract class ValueStubHook extends StubHook { return new PayloadStubHook(payload); })); } catch (err) { + // We took ownership of `args`, and there is no callee left to consume them. + args.dispose(); return new ErrorStubHook(err); } } @@ -1992,7 +2040,19 @@ export class PromiseStubHook extends StubHook { // can't serialize them yet, we have to deep-copy them now. args.ensureDeepCopied(); - return new PromiseStubHook(this.promise.then(hook => hook.call(path, args))); + return new PromiseStubHook(this.promise.then( + hook => { + try { + return hook.call(path, args); + } catch (err) { + args.dispose(); + throw err; + } + }, + err => { + args.dispose(); + throw err; + })); } stream(path: PropertyPath, args: RpcPayload): {promise: Promise, size?: number} { @@ -2000,10 +2060,19 @@ export class PromiseStubHook extends StubHook { // No size is returned because we can't know yet; this means the caller will await the promise, // which is the safe default (serialized writes). args.ensureDeepCopied(); - let promise = this.promise.then(hook => { - let result = hook.stream(path, args); - return result.promise; - }); + let promise = this.promise.then( + hook => { + try { + return hook.stream(path, args).promise; + } catch (err) { + args.dispose(); + throw err; + } + }, + err => { + args.dispose(); + throw err; + }); return { promise }; } @@ -2056,15 +2125,11 @@ export class PromiseStubHook extends StubHook { } dispose(): void { - if (this.resolution) { - this.resolution.dispose(); - } else { - this.promise.then(hook => { - hook.dispose(); - }, err => { - // nothing to dispose - }); - } + // Keep disposal behind calls already queued on this promise. Unlike pull(), dup(), and + // onBroken(), dispose() must not take a fast path through `this.resolution` even once it is + // available: it is the one destructive operation here, and a call chained on the promise just + // before disposal would otherwise be delivered after its target was disposed. + this.promise.then(hook => hook.dispose(), () => {}); } onBroken(callback: (error: any) => void): void { diff --git a/src/index.ts b/src/index.ts index 3d80501..a66a7d9 100644 --- a/src/index.ts +++ b/src/index.ts @@ -58,10 +58,19 @@ export const RpcStub: { * until you actually `await` the promise (or call `then()`, etc. on it). This is an optimization: * if you only intend to use the promise for pipelining and you never await it, then there's no * need to transmit the resolution! + * + * You may also construct an `RpcPromise` yourself from a regular `Promise`, using + * `new RpcPromise(promise)`, allowing you to perform promise pipelining on a local promise. This + * is semantically identical to creating a local-loopback RPC that returns the promise, and then + * invoking it: pipelined calls wait until the promise resolves, then are delivered, in order, to + * the resolution. This is useful when you plan to obtain some stub in the future, but want to + * allow code to start queuing calls on it immediately. Note that the `RpcPromise` takes + * ownership of the resolution: disposing it disposes the resolution, so resolve the promise + * with a `dup()` if you also intend to keep the stub. */ export type RpcPromise> = RpcPromiseType; export const RpcPromise: { - // Note: Cannot construct directly! + new >(value: Promise>): RpcPromise; } = RpcPromiseImpl; /**