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

Make SEXP both Send and Sync #202

Closed
CGMossa opened this issue Nov 12, 2023 · 4 comments
Closed

Make SEXP both Send and Sync #202

CGMossa opened this issue Nov 12, 2023 · 4 comments

Comments

@CGMossa
Copy link
Member

CGMossa commented Nov 12, 2023

By way of extendr/extendr#659, I think we should make SEXP a wrapper around SEXP, and define Send and Sync for this. It is a pointer, and I see no qualms with making it passable to threads. A higher-level abstraction like Robj should ensure that Send/Sync makes sense, through various means (like Box, Mutex, single_threaded, ownership, etc.).

@yutannihilation
Copy link
Contributor

Rustonomicon says:

raw pointers are neither Send nor Sync (because they have no safety guards).

@CGMossa
Copy link
Member Author

CGMossa commented Nov 14, 2023

Yes. But that's for rust types. Types that inherently contain information on whether they are Sync or Send. So you can stipulate with the abstractions mentioned above, what can happen to them.

But SEXP are with no type information. Either we allow them to be used as primitives, that then the higher level api can dress however it see fits, or this wrapping will happen in every other higher end library.

This eases the syntax burden upstream.

Like you wouldn't need to use usize in ownership module.

I can be persuaded that this is a bad idea though, I just want to communicate this opinion.

@yutannihilation
Copy link
Contributor

I might be wrong, but, in my understanding, pointers are not Sync or Send simply because there's no mechanism to prevent data race or access conflict. For example, it's possible you access to an SEXP and it's already GC-ed on another thread.

Like you wouldn't need to use usize in ownership module.

Yeah, I get your point that marking SEXP as Send and Sync is less bad than treating it as usize. But, I think it should be done rather on extendr's side by wrapping it with a new type if needed.

@CGMossa
Copy link
Member Author

CGMossa commented Nov 16, 2023

Thank you. I'd like to discuss this some more. I'm still myself not convinced of either direction.
I've this PR, extendr/extendr#666, that shows how wrapping SEXP to have these properties
would look like. This works, but the disadvantages are that every time the R-API is to be used, then you need a sexp.0 / sexp.inner(), etc.

Plus where types are defined, in this case libR-sys, is where things like Send/Sync/Copy/etc. ought to be deifned.

The GC example you mention is interesting. There is only one R-session active. So in a multi-threaded setting,
we'd have either one protection-mechanism, or a proteciton-mechanism per thread. I realise this while writing this
PR extendr/extendr#674, where I'd like to see, how extendr would look like, if we gave up,
on mult-threaded safety. In this case, I'd imagine either sticking to a common (amongst threads) protection mechanism, or a pr. thread protection mechanism, and that if they "share" resources, then they'd basically have said object in both their threads' ownership or just in the common one. Right now, the common protection insures that nothing gets deallocated unless all threads are done with it. And in a pr. thread protection, you'd need the SEXP to be Send, to uphold this as well..

A gc call happening while Rust is running will have no effect on whether an SEXP is still valid or not, because Rust doesn't decide what gets to gc'ed, without the above mentioned influence. This is due to having one common R-session.

But in any of these cases, you'd be sending SEXPs. In the multi-threaded version or not.

So in both implementations, you'd need SEXP to be Send. And in both you'd have to make sure that this make sense for the implementation. And in both you'd benefit from not having redundant sexp.0/sexp.inner() everywhere.
Usually, this is achieved by implementing Deref/DerefMut, but these don't make sense for pointer-things, as none of the R-API wants a reference to a pointer.. So the usual Rust way of dealing with new-type mania fails here.

I'd also emphasize that !Send and !Sync is not agnostic choices. It isn't necessarily "better" to have them be like that, for no good reason.

While that's said, I still don't want this to be forced in, without extensive discussions. The PRs I've made is to explore these ideas.

SIDE NOTE

Just another completely different thing, since SEXPREC is an opaque type, maybe what we should be doing is this: https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs ?

@CGMossa CGMossa closed this as completed Apr 22, 2024
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

No branches or pull requests

2 participants