Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Send promise resolution messages #471

Merged
merged 16 commits into from
Mar 16, 2023
Merged

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Feb 27, 2023

This patch updates sendCap such that it checks whether it should send senderHosted or senderPromise for an export. If the latter, it also arranges for a Resolve message to be sent when the promise is resolved.

Still TODO:

  • We should look at incoming unimplemented messages and drop the resolved reference if the remote peer indicates it doesn't understand the Resolve (as is the case with go-capnp currently) -- otherwise we will leak caps.
  • We need to actually test this properly; it doesn't break existing tests, but also none of the new code is covered by them.
  • We should provide a decent API for creating and then fulfilling promises.
  • We should start treating incoming senderPromise caps differently from senderHosted; we should rig up a promise for them, and then fulfill the promise when a Resolve comes in, rather than just ignoring it (responding unimplemented) as we do now.

Note: this patch adds a helper method to marshal an error into an Exception.

@zenhack zenhack marked this pull request as draft February 27, 2023 03:53
rpc/answer.go Outdated Show resolved Hide resolved
rpc/export.go Outdated Show resolved Hide resolved
rpc/export.go Outdated Show resolved Hide resolved
rpc/export.go Outdated Show resolved Hide resolved
@zenhack
Copy link
Contributor Author

zenhack commented Mar 1, 2023

Added a (failing) test. Stay tuned.

Leaks a client; need to investigate.
lthibault
lthibault previously approved these changes Mar 1, 2023
@zenhack
Copy link
Contributor Author

zenhack commented Mar 12, 2023

Test is fixed.

@zenhack zenhack marked this pull request as ready for review March 12, 2023 02:21
@zenhack
Copy link
Contributor Author

zenhack commented Mar 12, 2023

Tacked on another commit to handle a remote peer that doesn't understand resolve messages; without this, we'd leak caps.

@zenhack zenhack changed the title WIP: Send promise resolution messages Send promise resolution messages Mar 12, 2023
...which hangs. Some debugging suggests that this *is* being removed
from the table, but I need to figure out why the cap isn't actually
released.
lthibault
lthibault previously approved these changes Mar 12, 2023
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Note the failing CI though.

@zenhack
Copy link
Contributor Author

zenhack commented Mar 12, 2023

Yeah, I need to figure out why that test is failing -- I did some debugging, and the export is definitely being popped from the table, so I'm not sure why Shutdown() isn't actually being called. I'll keep you posted.

At some point in the past, Resolver was called Fulfiller; this is left
over from that.
@zenhack
Copy link
Contributor Author

zenhack commented Mar 16, 2023

Alright, last commit fixes the test.

We need to release this reference *before* the end of the function.
I really feel like Fulfill should take ownership here...
Copy link
Collaborator

@lthibault lthibault left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@lthibault lthibault merged commit e6a1eec into capnproto:main Mar 16, 2023
@zenhack zenhack deleted the senderPromise branch March 16, 2023 17:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants