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

Why does rpc.Transport care about the CapTable? #216

Closed
zenhack opened this issue Jan 10, 2022 · 7 comments
Closed

Why does rpc.Transport care about the CapTable? #216

zenhack opened this issue Jan 10, 2022 · 7 comments
Labels

Comments

@zenhack
Copy link
Contributor

zenhack commented Jan 10, 2022

I'm hoping @zombiezen can enlighten me on this one:

The rpc.Transport interface's NewMessage and RecvMessage methods both document that the caller is allowed to modify the cap table, but that it must be nil when sending or releasing the message. My question is: why? as far as I can tell this isn't actually relied upon by any actual code, except for the parts of the test suite that enforce it by panicking if it is non-nil at the wrong time.

This came up as I discovered when debugging #213 that a promise was erroneously being resolved to a nil Client, because this had been cleared for the benefit of send(). I'm staring at how to to rework this to avoid this problem, but I'm also wondering if the right solution is to just relax that requirement and then not clear the table. So I wanted to ask if there was a rationale, because it isn't obvious to me.

@zenhack
Copy link
Contributor Author

zenhack commented Jan 11, 2022

I am able to confirm that just not clearing cap table in extractCapTable fixes the problem I was having with #213, and all of the tests still pass if I comment out the explicit if CapTable != nil { panic(...) }.

@zenhack
Copy link
Contributor Author

zenhack commented Jan 13, 2022

@lthibault, I think if we don't hear from @zombiezen in a few days time I'll go ahead with a pr that gets rid of this requirement -- unless you see a reason not to. Curious as to your thoughts on the design.

@zombiezen
Copy link
Contributor

This is on my radar but I haven't had time to sit down and page in context. Will try to get back to you soon, but don't mark me as critical dependency. 👍

@zombiezen
Copy link
Contributor

Okay, I remember now. It's a question of ownership/lifetimes: since NewMessage and RecvMessage both return explicit destructors, having a cap table on the messages opens the question of who is responsible for releasing the capabilities in the table: the caller or the Arena. I suspect that I had run into an ordering or locking problem at some point, but I haven't been able to reconstruct that in my head. I'll bet that rather than try to specify behavior, I just decided it would be simpler for the caller to clear or move the cap table before calling the ReleaseFunc.

That said, now I'm curious as to what situation you were in where you were holding onto a cap table on a released or about-to-be-released message.

@zenhack
Copy link
Contributor Author

zenhack commented Jan 26, 2022

The problem really is that the CapTable needed to be empty when sending, not just releasing. The rpc system was working around this for rpc.answer.results by storing the CapTable in a separate resultCapTable field.

This became a problem when implementing support for receiverAnswer cap descriptors; I was hitting an error where some client was erroneously resolving to nil because the captable for a result had been cleared. It's possible I could have worked around it, but simply not clearing the cap table simplifies things.

Getting rid of resultCapTable entirely is still on my TODO list; it should be possible and should simplify things, but my first stab at it didn't work quite right and I haven't gotten back to it.

@zenhack
Copy link
Contributor Author

zenhack commented Feb 20, 2022

Thinking about this again, maybe the right semantics is to relax this so that CapTable may be non-nil when sending, but still require it to be nil when releasing, which solves the problem you were trying to address without making things awkward in my scenario.

@zenhack
Copy link
Contributor Author

zenhack commented Oct 9, 2022

We recently merged a patch that clarifies that the transport is supposed to .Release() anything in the cap table when the release func is called, which simplifies some things and happens to be existing behavior. So I'm to say this is settled. Closing.

@zenhack zenhack closed this as completed Oct 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants