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

Use refcounting for results #374

Merged
merged 8 commits into from
Dec 8, 2022
Merged

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented Dec 7, 2022

Stacked on top of #373.


There is no functional change here yet; this just factors the recounting
we were already doing into a package, and exposes the ability to
refcount to other parts of the code.

This is a stepping stone towards addressing #349; the idea is that the
method receiver should get a reference to this too to keep it alive for
the right amount of time.

In particular:

- resultsCapTable has been vestigial since we stopped clearing the
  CapTable on send, and I've been meaning to delete it. Instead, we just
  use the message's cap table directly, which also simplifies some code.
- We no longer need the code that releases Clients from the result's
  CapTable -- this is done by msg.Reset when we free the message. In
  principle, this shouldn't have hurt since Client.Release() is
  idempotent, but the way we were doing it caused the data race in capnproto#352
  Instead, we only need to release the additional items in exports if
  releaseResultCaps = true.
There is no functional change here yet; this just factors the recounting
we were already doing into a package, and exposes the ability to
refcount to other parts of the code.

This is a stepping stone towards addressing capnproto#349; the idea is that the
method receiver should get a reference to this too to keep it alive for
the right amount of time.
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.

I think this is good, but I'd like to take one last look at it after #327 & #373 has been merged, and after this has been rebased.

One small suggestion inline.

internal/rc/rc.go Show resolved Hide resolved
@zenhack
Copy link
Contributor Author

zenhack commented Dec 8, 2022

Merged #373 back into this.

These were added after we branched, so they need updating too.
@zenhack zenhack merged commit 2924ffa into capnproto:main Dec 8, 2022
@zenhack zenhack deleted the refcount-results branch December 8, 2022 23:38
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