autocxx icon indicating copy to clipboard operation
autocxx copied to clipboard

628-thread-safe-subclass-mode: shared pointer abstraction

Open tychedelia opened this issue 1 year ago • 5 comments

Parent issue: #628

Wanted to open this as a draft to get feedback before going any deeper. Using some ugly trait shenanigans to paper over Rc<RefCell<T>> and Arc<RwLock<T>>.

Outstanding comments/questions/issues:

  • Is this approach okay?
  • What's the best way to expose this to users? My initially approach was to duplicate the subclass proc macro which works fine for that bit, but I have no idea the best way to propagate this choice into codegen. Hard coded Rc<RefCell<T>> there for now just to get tests to pass. I think there might be a better API here, or at least need advice on how best to approach this.
  • Well want some more tests obviously.
  • Depends on RPITIT but that shouldn't be a problem soon.

As an aside, I'm using the subclass functionality for a project and it is literally lifesaving. This feature would be helpful for ensuring additional safety since I'm interfacing with a parent application's plugin API that does #whoknows with references we give them.


TODO:

  • [ ] Docs
  • [ ] Add more tests
  • [ ] Examples

tychedelia avatar Oct 25 '23 07:10 tychedelia

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

google-cla[bot] avatar Oct 25 '23 07:10 google-cla[bot]

Hi, first thoughts:

  • This is pretty epic! Thanks for diving so deep into this weird code and figuring it out.
  • Overall approach LGTM
  • I think #[subclass_multithreaded] sounds good to me
  • I'm not happy about RPITIT, I'm trying very hard to avoid unstable features even though they would make so much of autocxx easier...

adetaylor avatar Oct 25 '23 07:10 adetaylor

Thanks for the quick feedback! ✨

I think #[subclass_multithreaded] sounds good to me

Okay, I'll dig in deeper into the engine stuff and see where to go from here.

I'm not happy about RPITIT, I'm trying very hard to avoid unstable features even though they would make so much of autocxx easier...

Yes, hopefully by the end of the year in 1.75.0. But, I'll give it a shot on stable for now!

tychedelia avatar Oct 25 '23 08:10 tychedelia

Marking this ready for review, noting the pending todos listed above. Would love any feedback on the implementation before I move onto those housekeeping items.

I must admit, the programming model of this crate breaks my brain a bit when it comes to thinking about how to document the possibility for deadlocks, but I think writing some more tests will help here.

tychedelia avatar Oct 25 '23 20:10 tychedelia

I think there's a bit of trickiness here relating to the chosen ownership model and whether code generated by generate_subclass_fn should deadlock or panic.

  • In the case Rust owns the peer, calling try_* is helpful to prevent deadlocks and provide a good error message on re-entrant calls.
  • However, when C++ owns the peer, using try_* will just lead to panics if you try to actually do anything concurrently.

The safest thing to do is just accept the possibility of deadlocks. However, I think it might still be possible to check what kind of holder we have first and provide the good error message in the event we are calling a mutable receiver with an owned peer only. Will have to think more about this.

tychedelia avatar Oct 27 '23 04:10 tychedelia