tizoc / ocaml-interop

OCaml<->Rust FFI with an emphasis on safety.
MIT License
135 stars 22 forks source link

Runtime::init is broken if called multiple times #39

Open mrmr1993 opened 3 years ago

mrmr1993 commented 3 years ago

For example,

fn call_first() {
  let r1 = Runtime::init();
  ...
}
fn call_snd() {
  let r2 = Runtime::init();
  ...
}
pub fn main() {
  call_first();
  call_snd();
}

Currently, the second call will be unable to use the OCaml runtime, because it has been shutdown at the end of the first call.

It seems caml_startup and caml_shutdown already handle this correctly: startup increments a counter, shutdown decrements it, and the runtime is only started or stopped when that counter is 0. Removing the std::sync::Once in Runtime::init_persistent would restore this built-in behaviour, which seems to be strictly an improvement and fix this issue.

tizoc commented 3 years ago

@mrmr1993 that is correct, but it is exactly the behavior you get from caml_startup and caml_shutdown from the OCaml runtime, and I don't think there is a good way to fix it. BUT an improvement here would be to track that a shutdown has happened and panic if init is called again.

See here at the end of the section:

Once a runtime is unloaded, it cannot be started up again without reloading the shared library and reinitializing its static data. Therefore, at the moment, the facility is only useful for building reloadable shared libraries.

tizoc commented 3 years ago

I am even thinking that there is no need to track shutdowns, calling init more than once is just an error here, because you shouldn't ever have more than one owned instance of the runtime handle. So any call to init() after the first should panic.

tizoc commented 3 years ago

I guess it should better return a Result instead of triggering a panic to let the caller decide what to do. It is an api-breaking change so probably good to leave that to the split if things end being done that way.