Repository navigation
Conversation
When a call's arguments are an owned payload, the serializer only dups the payload's stubs or cites their import IDs, and sendCall() never disposed the payload on success. That is the case for a call forwarded to a remote stub (three parties, or a .map() callback) and for a call made through a pending RpcPromise. An RpcTarget passed this way never had its disposer run, and its table entries lived as long as the session. #241 covered the failure paths; this is the success path. Dispose the payload after the message is sent. Disposing can release an import the message cites, so doing it earlier makes the peer abort with "no such entry on exports table". App params don't own their stubs, so for them this does nothing. sendStream() gets the same line to honor the stream() ownership contract, though no public path delivers an owned payload there today. Disposing forwarded arguments can dispose an imported promise nobody has pulled yet, which rejected an unobserved promise (#293). Give activePull a no-op catch; real waiters still see the rejection.
🦋 Changeset detectedLatest commit: c615b65 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: |
![clanky-the-clanker[bot]](https://avatars.githubusercontent.com/in/5306670?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Approved — No issues found. Arguments are now disposed after send(), which releases owned payloads and leaves app-supplied params unchanged.
The decision is advisory: this review is a comment and never approves, blocks or requests changes on the pull request.
✅ All checks passed
🚀 Release: ✅ LGTM
Risk: Low. The changeset entries match the fix.
🔒 Security: ✅ LGTM
No issues found.
📖 AGENTS.md: ✅ LGTM
No issues found.
📋 Code Quality: ✅ LGTM
No issues found.
⚡ Performance: ✅ LGTM
No issues found.
📝 Documentation: ✅ LGTM
No issues found.
Reviewed commit: c615b65e
🚀 Deploying Preview to Cloudflare 🚀Preview URL: https://fix-disposal-bugs.pr.capnweb.com (commit c615b65)This URL reflects your latest Preview deploymentPreview Deployments by commit
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stubs passed as call arguments were never released when the call was re-sent from an owned payload: a call forwarded to a remote stub (three parties, or a
.map()callback), or one made through a pendingRpcPromise. The serializer only dups the payload's stubs or cites their import IDs, andsendCall()never disposed the payload on success. AnRpcTargetpassed this way never had its disposer run, and its table entries lived as long as the session. #241 covered the failure paths; this is the success path.Fixes #293, which the new dispose would otherwise hit on a successful forwarded call.
argsaftersend(), not after serializing. Disposing can release an import the message cites; done earlier, the peer aborts withno such entry on exports table.catchonactivePull, as Unhandled rejection when a call fails before delivery and an argument is an imported promise #293 suggests. An already-pulling import disposed before anyone pulls rejected a promise nobody observed. Real waiters still see the rejection.sendStream()to honor thestream()ownership contract. No public path delivers an owned payload there today, so no test.new RpcPromise(promise).