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

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

Open
littledivy opened this issue Jun 19, 2024 · 1 comment · Fixed by #791
Open

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

littledivy opened this issue Jun 19, 2024 · 1 comment · Fixed by #791

Comments

@littledivy
Copy link
Member

littledivy commented Jun 19, 2024

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

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:

  • disallow &T in async ops
  • ensure the same value can not have both a &mut T and &T reference within a single sync op
  • ensure the sync op is not reentrant

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.

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 a pull request may close this issue.

2 participants