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

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

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

tychedelia
Copy link

@tychedelia tychedelia commented Oct 25, 2023

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

@google-cla
Copy link

google-cla bot commented Oct 25, 2023

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.

@adetaylor
Copy link
Collaborator

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...

@tychedelia
Copy link
Author

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 tychedelia marked this pull request as ready for review October 25, 2023 20:29
@tychedelia
Copy link
Author

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
Copy link
Author

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.

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.

None yet

2 participants