denoland / deno_core

The core engine at the heart of Deno
MIT License
232 stars 75 forks source link

Allow `#[cppgc] &mut T` in sync ops #787

Open littledivy opened 2 weeks ago

littledivy commented 2 weeks ago

This will greatly simplify Zlib and net.Blocklist in Deno as we have to wrap them in RefCell for mutability. Should be pretty easy to implement

lucacasonato commented 2 weeks ago

Copying over my comments from #793:

I am not sure we can guarantee any safety at all with this:

struct Wrap(usize);

#[op2(fast)]
async fn op_read(#[cppgc] wrap: &Wrap) {
  println!("foo start: {}", wrap.0);
  tokio::time::sleep(Duration::from_secs(1));
  println!("foo end:   {}", wrap.0);
}

#[op2(fast)]
fn op_write(#[cppgc] wrap: &mut Wrap) {
  wrap.0 += 1;
  println!("bar:       {}", wrap.0);
}
const wrap = op_make_wrap();

const promise = op_read(wrap); // foo start: 0
op_write(wrap);                // bar:       1
await promise;                 // foo end:   1

Here we are explicitly violating one of the Rust memory safety axioms: a user must not be able to modify a value, while someone has an immutable reference (&Wrap), to that value.

I don't think this is fixable by making local changes to what are valid &mut T values in an op. You'd have to:

And further, &mut T may need to be Pin<&mut T> (this I am less sure about).

To then get access to cppgc values in async ops, one would not get a reference, but something like a Gc<T>. This Gc struct would be ?Send. The struct would have a borrow() / borrow_mut(). The value returned from these would be GcRef<T> / GcRefMut<T> structs that are also ?Send. They deref to &T / &mut T. When calling borrow() we'd have to ensure at runtime that there is currently no borrow_mut() ongoing (and vice versa). You can see, this looks an awful lot like a RefCell (with likely similar perf considerations).

That also does not seem viable, because we are now adding the overhead of RefCell to all cppgc objects.

So I have a final proposal that I think solves both problems: we have two types of GcResource, that are mutually exclusive: immutable resources, and sync mutable resources. These would work as follows: immutable resources could only have immutable references taken, but both in sync and async ops. Mutable references could have both immutable and mutable references taken, but only in sync ops (and not both at the same time in a single op). An example: https://gist.github.com/rust-play/d3a0afca92d2b4ed30436712cfc62dd8

These invariants can be enforced using the compiler (except for the "no mutable and immutable borrow for the same type in a single sync op", and the requirement that the op must not be reentrant.

And following on from that: if you do need a mutable resource in an async op, you can use an immutable resource containing a RefCell.