fix: dispose call args on all failure paths per new StubHook ownership contract - #241
fix: dispose call args on all failure paths per new StubHook ownership contract#241ndisidore wants to merge 6 commits into
Conversation
🦋 Changeset detectedLatest commit: 7e7bea9 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.
|
One semantics question I hit while in here: if a call's args contain unresolved promises, Pre-existing stuff: this PR just keeps disposal from overtaking forwarding, same as calling the destination hook directly. Is that intended, or should disposal also wait for in-flight deliveries? (That'd be a |
|
/bonk review this |
77ff252 to
1cd41dc
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
/bonk review this |
1cd41dc to
ed803fe
Compare
…ind queued calls PromiseStubHook.call() and .stream() deep-copy their arguments before chaining on the backing promise, but if that promise rejects, the copies were never disposed, leaking any stubs they contained. PromiseStubHook.dispose() also had a fast path that disposed the resolution synchronously once available. A call chained on the promise just before disposal could then be delivered after its target was already disposed, violating ordering. Disposal now always chains on the promise so it stays behind previously queued calls.
ErrorStubHook.call() ignored the arguments it takes ownership of, and ErrorStubHook.map() likewise ignored its captures, so anything forwarded to a broken or disposed hook leaked. ValueStubHook.call() had the same gap on its error path (its own map() already disposes captures there), and PromiseStubHook's forwarding continuations did not clean up when the destination hook threw synchronously.
Co-authored-by: Kenton Varda <kenton@cloudflare.com>
… args Document on the abstract StubHook that call(), stream(), and map() take ownership of their args/captures even on synchronous throw, and fix the implementations that violated it: - RpcImportHook.call()/stream(): dispose args if getEntry() throws, and in sendCall()/sendStream() when aborted or when argument serialization fails (safe: exported hooks are dups and the Devaluator rolls back its exports). - Default StubHook.stream(): dispose the result hook if pull() throws. - MapVariableHook and the map-not-loaded placeholder: dispose args/captures before throwing. - ValueStubHook.call(): restructure to the inner-catch pattern so delegation clearly hands ownership to the delegate. - PromiseStubHook: drop the success-path try/catch around hook.call()/ hook.stream() -- per the contract the resolved callee owns the args; keep disposal only on rejection, where no callee ever existed.
b9f79da to
c93a265
Compare
- RpcImportHook: collapse the three hand-rolled getEntry() guards in call()/stream()/map() into a private getEntryTakingOwnership() helper. - ValueStubHook.map(): flatten the nested try/catch into a single catch that disposes captures defensively (dispose() is idempotent), matching call()'s flat shape. - Tests: SyncThrowingHook extends ErrorStubHook instead of hand-stubbing all eight abstract StubHook methods. - Condense the changeset to changelog style.
c93a265 to
a8be070
Compare
- Revert the mapImpl placeholder change: the stubs are always replaced at startup, so handling disposal there is dead code. - ValueStubHook.map(): restore the narrow inner catch. Once ownership of the captures transfers to the delegate or applyMap(), disposing them in the outer catch is incorrect (a partially-completed callee may hold live references, e.g. hooks stored in the export table). - Changeset: drop implementation details from the changelog entry.
RPC call arguments (and map captures) leaked whenever a call failed before reaching a callee. A rejected pipeline promise, a broken or disposed stub, a failed argument serialization, or a sync throw inside a hook.
Rather than patching each site with caller-side try/catch (as an earlier revision of this PR did), the fix defines the rule once on the abstract
StubHook:call(),stream(), andmap()take ownership of their args/captures even when they throw synchronously, and callers never dispose after invoking.The implementations that violated this are fixed
RpcImportHook.call()/stream()(including mid-serialization failures, where disposal is safe because exported hooks are dups and the Devaluator rolls back its exports)stream()(which leaked the result hook whenpull()threw)MapVariableHookmapplaceholderPromiseStubHooknow disposes its copied args only when its backing promise rejects i.e. where no callee ever existed to take them.Also kept from the original PR:
PromiseStubHook.dispose()chains on the backing promise so disposal stays ordered behind already-queued calls.