Should JsClass be ParallelSend

[Sytten]

Hi!
It hit me that JsClass was not Send if the parallel is enabled.
I have an intuition that this is an issue since the classes can then own non-send struct like Rc and that would lead to bugs since the runtime can change threads.
In particular in LLRT we do use a lot of Rc<RefCell> and they might need to be changed to Arc<Mutex>?
What are you thoughts?

[DelSkayn]

This is a somewhat difficult topic which I have been thinking about a lot.

So the current idea is that you can see all the closure passed to the with like functions act as a barrier. Only Send types should be able to move across that barrier and nothing from inside that is not Send or is a js like object can move outside of the barrier.

This (mostly) should allow non-Send types to be created and used inside the with closure safely as the runtime lock prevents shared access from multiple threads. So having RC is fine as you can never access the Rc value from different threads at the same time.

However this still technically breaks the contract of Send as the entire lock can be moved to a different thread with the non-send types inside. This isn't a problem for most non-send types like RC however for non-send types which are locked to a specific thread it is a problem. If a type relies on thread-local storage of a specific thread or are locked to the main thread, like window system event loops, and are used in the with closure with parallel enabled it can cause unsound code.

I don't have a good solution to the above problem, all the rquickjs types need to be non-send (even with parallel enabled) for safety and need to be usable within the with closure. The only way we can have this and allow parallel access behind a lock is the current implementation, otherwise the functionality parallel provides is just not possible safely.

[Sytten]

Right that makes sense.

I do still have couple ideas:

• We should heavily document that behaviour since it is really not obvious. Specially if someone wants to use the Outlive mechanism that crosses the with barrier.

• The fact that RC works is kinda an implementation detail that could change in later versions of Rust. So we need I think some way of expressing this.

• Maybe we could have our own marker trait that would be called Sendable or something. Implemented by default for all Send struct and unsafe to implement on others like Rc. At minimum that would force a class designer to think about it but still allow enough flexibility I think.

• We might want to consider an optional model ala sqlx where the event loop is on a different thread and we send messages to it. That would allow us to remove most of the mutexes and synchronization we currently have to do.

[Sytten]

Just for reference:

/// Since rquickjs uses a global lock, we can be more lax then the Send
/// trait but you should still make sure that objects are safe to send between
/// threads (for example they should not use thread locals).
pub unsafe trait Sendable {
}

#[cfg(feature = "parallel")]
unsafe impl<T: Send> Sendable for T

#[cfg(feature = "parallel")]
unsafe impl<T> Sendable for Rc<T>

#[cfg(not(feature = "parallel"))]
unsafe impl<T> Sendable for T

[DelSkayn]

I tried something like this before and after trying it today I found out why I abandoned adding such a marker trait.
The problem is that implementing unsafe impl<T: Send> Sendable for T prevents any type which is not Send from implementing the trait. The compiler will hit you with:

error[E0119]: conflicting implementations of trait `CaptureSend` for type `Class<'_, _>`
  --> core/src/markers.rs:93:1
   |
91 | unsafe impl<T: Send> CaptureSend for T {}
   | -------------------------------------- first implementation here
92 |
93 | unsafe impl<'js, C: CaptureSend> CaptureSend for Class<'js, C> {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation for `Class<'_, _>`
   |
   = note: upstream crates may add a new impl of trait `std::marker::Send` for type `rquickjs_sys::JSValue` in future versions
   = note: upstream crates may add a new impl of trait `std::marker::Send` for type `std::marker::PhantomData<*mut ()>` in future versions

I haven't been able to figure out a work around for this. impl !Send for Bla{} is still unstable. And adding a phantomdata with a type like *mut () which will obviously never be Send still hits you with the error.

So if we want such a type it would need to implemented it for every type in std manually, every external crate type that could be used with rquickjs and the user would need to implement it for every type they might want to use with rquickjs which I consider to much of a hassle.

So we are somewhat stuck here.

[Sytten]

Right I forgot about the orphan rule, we will have to wait for rust-lang/rust#29864 I think to be stabilized. Let's keep the issue open I guess? Maybe we can label it as "enhancement".