search

rusterlium / erlang_nif-sys

Low level bindings to Erlang NIF API for Rust
Apache License 2.0
90 stars 19 forks source link

resource mutability #11

Open goertzenator opened 8 years ago

goertzenator commented 8 years ago

While working on Ruster, I've discovered that the resource functions in erlang_nif-sys may have the wrong mutability:

fn enif_alloc_resource(_type: *mut ErlNifResourceType, size: size_t) -> *mut c_void
fn enif_release_resource(obj: *mut c_void)
fn enif_get_resource(arg1: *mut ErlNifEnv, term: ERL_NIF_TERM, _type: *mut ErlNifResourceType, objp: *mut *mut c_void) -> c_int
fn enif_keep_resource(obj: *mut c_void)

I think the object pointers in all except enif_alloc_resource should be changed to *const c_void. Consider the scenario where two Erlang processes have the same resource term and simultaneously call Nifs that manipulate that resource. Having a mutable pointer from enif_get_resource will enable concurrent mutation (bad!). The user can be steered in a safer direction by providing a const pointer. Interior mutability can still be achieved with Cell, RefCell, and locks.

The underlying C NIF API is compelled to use mutable pointers here because resource refcounts need to be mutated. C can't hide these things under interior mutability while staying exteriorly immutable. With this Rust wrapper we have an opportunity to hide such implementation details.

@hansihe, would this change cause breakage for Rustler?

hansihe commented 8 years ago

Seems like a sensible change.

It might cause breakage, but it would be trivial to fix.

jorendorff commented 7 years ago

This change was made in 0.5 (rev f801f2e), so this can be closed.

I was surprised when I saw this discrepancy between the C and Rust function signatures — it's neat to see the reasoning behind it. ...Unfortunately, I don't see a good way to communicate this to users. Putting it in a doc comment wouldn't help, because users quickly learn not to look at the rustdoc, but to look at the erl_nif docs instead.

goertzenator commented 7 years ago

Oops, thanks. But maybe I'll leave this open as a reminder to eventually write a paragraph about it in docs. It could be part of a larger discussion on mutability, such as why environments are always *mut (because they are not thread safe) and other things.