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

Store ClientSnapshots in the CapTable. #525

Closed
wants to merge 13 commits into from

Conversation

zenhack
Copy link
Contributor

@zenhack zenhack commented May 27, 2023

Stacked on #522 and #524; review and merge those first. This makes the CapTable hang on to ClientSnapshots directly, which will be important for addressing the 4-way race in the context of answers.

zenhack added 13 commits May 27, 2023 15:00
I plan on adding *Snapshot versions and eventually storing snapshots
internally.
This seems like an unnecessary complication, and I don't want to deal
with it when porting stuff over to snapshots.
...and use it to catch a bug; the test is failing, need to track it
down.
Otherwise the target message steals the caps, resulting in errors
later when we try to read them.
Per capnproto#523, this is currently failing frequently. Mark it as flaky
until we're ready to actually deal with it.
In addition to letting us hand out borrowed references, Storing the
snapshots will facilitate code that needs them not to resolve further
after AddSnapshot() or SetSnapshot().
@zenhack zenhack marked this pull request as draft May 29, 2023 06:27
@zenhack
Copy link
Contributor Author

zenhack commented May 29, 2023

Marking this as a draft; fussing with this on one of the follow-up features has convinced me that this is a bit error prone: you end up with two things to remember to release, and I'm hitting a cascade of leaks in branch for #480. I may rethink how to do this, time to sleep on it.

@zenhack
Copy link
Contributor Author

zenhack commented May 29, 2023

I think instead of doing this, I'm going to leave CapTable as just storing Clients, and try bringing back the resultsCapTable field that used to be stored in rpc.answer; it simplified things a bit to get rid of it, but I think bringing it back as a slice of snapshots will be much less intrusive than trying to stick snapshots in Message's cap table. Closing.

@zenhack zenhack closed this May 29, 2023
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.

1 participant