feat: construct RpcPromise from a Promise - #242
Conversation
🦋 Changeset detectedLatest commit: 49d3b4e The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
commit: |
This comment was marked as outdated.
This comment was marked as outdated.
0535521 to
1ecfa82
Compare
1cd41dc to
ed803fe
Compare
1ecfa82 to
1065dbf
Compare
|
@ask-bonk review this |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — LGTM
Re-reviewed at 33c0122, which addresses all four of @kentonv's review comments. Verified locally: build succeeds, test:types passes, and all 178 tests in index.test.ts pass on node, chromium, and workerd.
Review feedback resolution
- Type overloads (
src/index.ts) — the two overloads are collapsed into a singlenew <T extends RpcCompatible<T>>(value: Promise<T | Stub<T>>): RpcPromise<T>, andPromiseLikewas dropped (non-Promisethenables no longer accepted, as agreed). Inference works without an explicit<T>— the type tests confirmnew RpcPromise(Promise.resolve(pointStub))infersRpcPromise<PointTarget>(__type-tests__/rpc-base-cases.test.ts:160-162). hookForResolutionone-liner — inlined.hookForPromiseArg— inlined into the constructor.- The bare-stub bug — the real catch. The branch is now gated on
typeForRpc(hook) === "rpc-promise"only (src/core.ts:555), so a bareRpcStubno longer has its non-promise hook adopted directly (which couldn't answerpull()); it falls through to the payload path instead. Covered by the regression test "resolves when awaited after construction from a bare local stub" (__tests__/index.test.ts).
Correctness
- Rejection is adopted as an
ErrorStubHookand the backingPromiseStubHookpromise never rejects; combined withignoreUnhandledRejections(), discarded pipelined calls, awaits, andmap()results can't leak unhandled rejections. All three "does not report an unhandled rejection…" tests pass. - Disposal is correctly ordered behind queued calls via
PromiseStubHook.dispose()— the "delivers a call initiated before disposal" and "disposes the eventual target when disposed before resolution" tests exercise exactly the ordering paths that rely on the #241 fixes, which justifies the stacking. - Adopting an existing
RpcPromisedirectly preserves laziness (only one["pull"]is sent) and brokenness — verified by the "keeps an adopted RpcPromise lazy" and "preserves brokenness" tests. - No wire-protocol change (client-side construction reusing existing representations), so no
protocol.mdupdate needed. Changeset present (minor). README documents the ownership-transfer and unbounded-queue caveats.
No logic bugs, capability leaks, or backward-compat concerns found. Ship it.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
| export type RpcPromise<T extends RpcCompatible<T>> = RpcPromiseType<T>; | ||
| export const RpcPromise: { | ||
| // Note: Cannot construct directly! | ||
| new <T extends RpcCompatible<T>>(value: Promise<T | Stub<T>>): RpcPromise<T>; |
There was a problem hiding this comment.
I don't understand why we need | Stub<T> here.
Doesn't it block type inference? It seems like it would.
Either way though, I don't see why it would be needed.
There was a problem hiding this comment.
Doesn't it block type inference? It seems like it would.
It does not and there's actually a test for this already in __type-tests__/rpc-base-cases.test.ts around line 156
Either way though, I don't see why it would be needed.
turns out is is needed
stubs aren't assignable to their target types (their methods return Results), so without | Stub<T> you can't pass a promise that resolves to a stub
without the union new RpcPromise<TestTarget>(Promise.resolve(stub.dup())) is a compile error.
per 🤖
The
| Stub<T>member is what lets TypeScript matchPromise<Stub<PointTarget>>againstStub<T>and pull outT = PointTarget
There was a problem hiding this comment.
It appears there are no test cases for new RpcStub(promise) where promise is type Promise<RpcStub<T>>.
This is what I'm confused about. In that case, is the result RpcPromise<T> or RpcPromise<RpcStub<T>>?
Note that these two types behave equivalently, but it seems like type deduction would have to choose one? Which does it choose?
I would actually lean towards RpcPromise<RpcStub<T>> being more correct -- and if that's the outcome we want, then I think then the | Stub<T> is not needed?
There was a problem hiding this comment.
Its not immediately obvious, but there is a test that covers this case here.
It chooses RpcPromise<T> and at runtime, you're right they behave equivalently, but the type system these aren't compatible.
Removing it would cause your own example in the readme to become a compile error and actually muddies up other types.
c93a265 to
a8be070
Compare
Resolves the long-standing TODO on the RpcPromise constructor: the application may now pass a Promise (or any other thenable) for the eventual resolution. Calls made before the promise settles are queued and delivered in order once it does, so an RpcPromise can stand in for a capability that doesn't exist yet -- for example, one that will only become available after a broken session has been re-established. The promise may resolve to an RpcTarget, a stub, or a plain value. Promise.resolve() performs thenable assimilation natively, so no hand-rolled hardening against misbehaving thenables is needed. The resolution is adopted with return semantics (the same representation used for resolutions of local async calls), so awaiting delivers the value, pipelined calls forward through it without forcing a pull, and brokenness of a stub resolution is preserved. Passing an existing RpcPromise adopts its hook directly, keeping it lazy. A rejection is adopted as an ErrorStubHook rather than left to reject the backing promise, so the promise chains behind queued calls never reject: calls land on the ErrorStubHook (which disposes their arguments) and the error surfaces only through pull() or onBroken(). Without this, a discarded pipelined call on a promise-backed stub would raise an unhandled rejection event when the promise rejects (crashing Node under its default handling), even though fire-and-forget calls on the session-backed stub it stands in for reject only on pull.
33c0122 to
11d58ca
Compare
- Only adopt the hook of an existing RpcPromise; a bare stub's hook may not implement pull(), so bare stubs now take the generic path, whose resolution payload handles them correctly (await previously rejected with "Tried to resolve a non-promise stub."). Regression test added. - Inline hookForPromiseArg and hookForResolution into the constructor. - Collapse the constructor's type overloads into a single signature, narrowing the accepted type to Promise (runtime still assimilates arbitrary thenables). - Reframe the README section around the local-loopback RPC equivalence, and align the jsdoc and changeset with it.
11d58ca to
8c51593
Compare
- Adopting an existing RpcPromise now consumes the source: its hook is neutered to DISPOSED_HOOK, so disposing the source can no longer silently kill the wrapper. Using the source after wrapping reports the standard disposed error. - Restore the invariant that every RpcPromise has a defined path by defaulting pathIfPromise to [] on the internal StubHook path. - Wrap workerd-native RpcPromise/RpcProperty values (rpc-thenable) in a TargetStubHook so pipelined calls aren't eagerly assimilated. - Document ownership transfer on adoption and the dup() workaround for keeping a deferred capability lazy when resolving a native Promise with an RpcPromise.
| // 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. | ||
| let raw = unwrapStubAndPath(<RpcStub><unknown>hook); |
There was a problem hiding this comment.
There was some discussion abut what happens when new RpcPromise(existingRpcPromise) adopts the source's hook. Opted for this consume-and-neuter approach (e.g. treated like a move) for consistency
practically this means a user who wants copy semantics writes new RpcPromise(source.dup()) themselves which is in line with convention for the rest of the lib
The alternative was something like
super(unwrapStubAndDup(<RpcStub><unknown>hook), []);to auto-dup which would leave the original promise usable.
I'm impartial, but what is here seems marginally more consistent
There was a problem hiding this comment.
I think taking ownership is consistent with what happens with Promise<T> -- the RpcPromise will take responsibility for disposing the T.
Worth noting, though, that dup() won't work here: it returns an RpcStub, even when called on an RpcPromise.
| // rejection event. | ||
| let promiseHook = new PromiseStubHook(Promise.resolve(hook).then( | ||
| value => new PayloadStubHook(RpcPayload.fromAppReturn(value)), | ||
| err => new ErrorStubHook(err))); |
There was a problem hiding this comment.
PromiseStubHook already handles the error case. We should not be handling it explicitly here.
From the comment above it sounds like this may have been done due to the missing disposal of arguments in that case -- but you've now fixed that.
| * Passing an existing `RpcPromise` to the constructor adopts it directly, transferring ownership: | ||
| * the original promise is consumed (as if disposed) and the wrapper must be used in its place. | ||
| * | ||
| * Note that if a regular `Promise` resolves to an `RpcPromise`, JavaScript's promise machinery |
There was a problem hiding this comment.
Delete this paragraph. What it's describing is not specific to the RpcPromise constructor -- it actually applies also when returning an RpcPromise from any async function or many other places. It's an interesting quirk but doesn't belong here.
Also the way the AI has described it here is probably inscrutable to most humans.
| * ownership of the resolution: disposing it disposes the resolution, so resolve the promise | ||
| * with a `dup()` if you also intend to keep the stub. | ||
| * | ||
| * Passing an existing `RpcPromise` to the constructor adopts it directly, transferring ownership: |
There was a problem hiding this comment.
I'd probably delete this paragraph too. It's describing a behavior that nobody would trigger intentionally -- if you know you have an RpcPromise, you would not bother constructing a new RpcPromise from it, you'd just use it as-is. So documenting the behavior is distracting.
| * If the promise rejects, the rejection propagates to all pipelined calls. | ||
| * etc. | ||
|
|
||
| Two special cases to be aware of: |
There was a problem hiding this comment.
Delete these two special cases, nobody actually needs to be aware of them.
2 participants
You can now write
new RpcPromise(promise)(resolves the TODO). Calls made before the promise settles are queued and delivered in order once it does, and awaiting it yields the resolution. The promise can resolve to anRpcTarget, a stub, or a plain value.@kentonv mentioned this re: reconnection in cloudflare-os#172
Passing an existing
RpcPromiseadopts its hook directly rather than awaiting it. Two caveats are documented in the README: ownership of the resolution transfers to theRpcPromise(resolve with adup()if you also keep the stub), and pending calls queue unboundedly (reject the promise when giving up).Stacked because the new tests exercise rejection and dispose-before-resolution paths that only behave correctly with the
PromiseStubHookfixes in #241. Landing that first keeps this diff feature-only.