search

capnproto / capnproto-rust

Cap'n Proto for Rust
MIT License
2.08k stars 222 forks source link

Support a MembranePolicy #234

Open griff opened 3 years ago

griff commented 3 years ago

I could really use something like MembranePolicy from membrane.h.

Any plans on adding it ever?

dwrensha commented 3 years ago

I don't have any immediate plans to add it. I agree that it seems useful.

griff commented 3 years ago

I did some work on just porting membrane.c++ to rust to get a feel for what is missing to support this feature.

The biggest thing missing is a trait for CapTable and it might need a change to Imbue and ImbueMut to use that trait instead. Since this would be a breaking change I thought I would talk to you first.

griff commented 3 years ago

I did some more work on this and have hit a bit of a roadblock: In order to override the cap table I either need to have the cap table be owned by the the readers and builders which makes them not support being Copy. Or I need to override them in the hooks where the cap table is currently owned which means I need to use something like a RefCell in the places where the cap table is in an Rc but that breaks get on the various hooks because to get to the PointerReader it would need to go through Ref which then is only borrowed for the life of the get call.

Do you have any ideas on how to get around these issues? And is it strictly necessary for the readers and builders to be Copy?

griff commented 3 years ago

I now have something that more or less works but it is not always pretty. To get around CapTable ownership problems I needed to make two changes to the hooks.

1) For Reader hooks like ParamsHook I copy the CapTable so that my membrane variants has a fully owned copy. 2) For Builder hooks like ResultsHook and RequestHook I had to add methods to swap out the CapTable with my membrane variant of the table and swap the cap table back again.

I still need to find a deadlock in one of the tests and port the membrane revoke test. But all other tests are ported and working. I might also wait a bit with the PR and use the membrane API for my own project to see if the ergonomics of the API needs improvement and wether some more of the API needs to be able to return an error.

griff commented 3 years ago

The cost is mostly bourn by users of membranes but I did have to change CapTable from Vec<Option<Box<dyn ClientHook>>> to a new Box<dyn CapTableHook> and I had to therefore wrap the Vec in a Box and that extra Box and the dynamic dispatch on CapTableHook is the cost for all users.