fix: Ensure that threads are dropped when using child threads

Uses a very naive approach to determine if there still exists thread
references by scanning every thread. Could be faster but this fixes the
immediate issue.

tests/api.rs

@@ -541,7 +541,7 @@ impl VmType for NoisyDrop {
 }
 
 #[test]
-fn cyclic_userdata() {
+fn cyclic_userdata_simple() {
     let _ = ::env_logger::try_init();
 
     #[derive(Debug, Userdata, Trace)]
@@ -639,3 +639,69 @@ fn cyclic_userdata_mutable() {
         "The virtual machine and its values were not dropped"
     );
 }
+
+#[test]
+fn child_vm_do_not_cause_undroppable_cycle_normal_drop_order() {
+    let _ = ::env_logger::try_init();
+
+    let mut noisy_drop = NoisyDrop::default();
+    {
+        let vm = make_vm();
+        let child_vm = vm.new_thread().unwrap();
+
+        vm.register_type::<NoisyDrop>("NoisyDrop", &[])
+            .unwrap_or_else(|_| panic!("Could not add type"));
+
+        Compiler::new()
+            .load_script(&vm, "function", "\\x -> ()")
+            .unwrap();
+
+        {
+            let mut f: FunctionRef<fn(NoisyDrop)> = vm
+                .get_global("function")
+                .unwrap_or_else(|err| panic!("{}", err));
+            f.call(noisy_drop.clone()).unwrap();
+        }
+
+        drop(child_vm);
+        drop(vm);
+    }
+
+    assert!(
+        Arc::get_mut(&mut noisy_drop.0).is_some(),
+        "The virtual machine and its values were not dropped"
+    );
+}
+
+#[test]
+fn child_vm_do_not_cause_undroppable_cycle_reverse_drop_order() {
+    let _ = ::env_logger::try_init();
+
+    let mut noisy_drop = NoisyDrop::default();
+    {
+        let vm = make_vm();
+        let child_vm = vm.new_thread().unwrap();
+
+        vm.register_type::<NoisyDrop>("NoisyDrop", &[])
+            .unwrap_or_else(|_| panic!("Could not add type"));
+
+        Compiler::new()
+            .load_script(&vm, "function", "\\x -> ()")
+            .unwrap();
+
+        {
+            let mut f: FunctionRef<fn(NoisyDrop)> = vm
+                .get_global("function")
+                .unwrap_or_else(|err| panic!("{}", err));
+            f.call(noisy_drop.clone()).unwrap();
+        }
+
+        drop(vm);
+        drop(child_vm);
+    }
+
+    assert!(
+        Arc::get_mut(&mut noisy_drop.0).is_some(),
+        "The virtual machine and its values were not dropped"
+    );
+}

tests/io.rs

@@ -1,13 +1,15 @@
-extern crate env_logger;
-extern crate gluon;
-extern crate tempfile;
-extern crate tokio;
-
-use gluon::vm::api::{Hole, OpaqueValue, OwnedFunction, ValueRef, IO};
-use gluon::{new_vm, Compiler, Thread};
 use std::fs;
+
 use tempfile::NamedTempFile;
 
+use gluon::{
+    new_vm,
+    vm::api::{Hole, OpaqueValue, OwnedFunction, ValueRef, IO},
+    Compiler, Thread,
+};
+
+use tokio::runtime::current_thread::Runtime;
+
 #[macro_use]
 mod support;
 
@@ -164,7 +166,7 @@ fn spawn_on_twice() {
         action
     "#;
 
-    let mut runtime = self::tokio::runtime::Runtime::new().unwrap();
+    let mut runtime = Runtime::new().unwrap();
     let vm = make_vm();
     let (result, _) = runtime
         .block_on(
@@ -210,7 +212,7 @@ fn spawn_on_runexpr() {
         wrap x.value
     "#;
 
-    let mut runtime = self::tokio::runtime::Runtime::new().unwrap();
+    let mut runtime = Runtime::new().unwrap();
     let vm = make_vm();
     let (result, _) = runtime
         .block_on(
@@ -249,7 +251,7 @@ fn spawn_on_do_action_twice() {
         wrap (load counter)
     "#;
 
-    let mut runtime = self::tokio::runtime::Runtime::new().unwrap();
+    let mut runtime = Runtime::new().unwrap();
     let vm = make_vm();
     let (result, _) = runtime
         .block_on(
@@ -282,7 +284,7 @@ fn spawn_on_force_action_twice() {
         wrap (load counter)
     "#;
 
-    let mut runtime = self::tokio::runtime::Runtime::new().unwrap();
+    let mut runtime = Runtime::new().unwrap();
     let vm = make_vm();
     let (result, _) = runtime
         .block_on(
@@ -314,7 +316,7 @@ fn spawn_on_runexpr_in_catch() {
         (io.catch action wrap >>= io.println) *> wrap "123"
     "#;
 
-    let mut runtime = self::tokio::runtime::Runtime::new().unwrap();
+    let mut runtime = Runtime::new().unwrap();
     let vm = make_vm();
     let (result, _) = runtime
         .block_on(

tests/serialization.rs

@@ -7,18 +7,22 @@ extern crate walkdir;
 
 extern crate gluon;
 
-use std::fs::File;
-use std::io::Read;
+use std::{fs::File, io::Read};
 
 use futures::Future;
 
 use crate::serde::ser::SerializeState;
 
-use gluon::vm::api::{Hole, OpaqueValue};
-use gluon::vm::serialization::{DeSeed, SeSeed};
-use gluon::vm::thread::{RootedThread, RootedValue, Thread, ThreadInternal};
-use gluon::vm::Variants;
-use gluon::{new_vm, Compiler};
+use gluon::{
+    new_vm,
+    vm::{
+        api::{Hole, OpaqueValue, IO},
+        serialization::{DeSeed, SeSeed},
+        thread::{RootedThread, RootedValue, Thread},
+        Variants,
+    },
+    Compiler,
+};
 
 fn serialize_value(value: Variants) {
     let mut buffer = Vec::new();
@@ -30,10 +34,10 @@ fn serialize_value(value: Variants) {
     String::from_utf8(buffer).unwrap();
 }
 
-fn roundtrip<'t>(
-    thread: &'t RootedThread,
+fn roundtrip(
+    thread: &RootedThread,
     value: &OpaqueValue<&Thread, Hole>,
-) -> RootedValue<&'t Thread> {
+) -> RootedValue<RootedThread> {
     use std::str::from_utf8;
 
     let value = value.get_variant();
@@ -51,7 +55,6 @@ fn roundtrip<'t>(
     let deserialize_value = DeSeed::new(thread)
         .deserialize(&mut de)
         .unwrap_or_else(|err| panic!("{}\n{}", err, buffer));
-    let deserialize_value = thread.root_value(unsafe { Variants::new(&deserialize_value) });
 
     // We can't compare functions for equality so serialize again and check that for equality with
     // the first serialization
@@ -61,6 +64,7 @@ fn roundtrip<'t>(
         let ser_state = SeSeed::new();
         value.serialize_state(&mut ser, &ser_state).unwrap();
     }
+    eprintln!("{}", buffer);
     assert_eq!(buffer, from_utf8(&buffer2).unwrap());
 
     deserialize_value
@@ -199,11 +203,23 @@ fn roundtrip_lazy() {
 }
 
 #[test]
-fn roundtrip_thread() {
+fn roundtrip_std_thread() {
     let thread = new_vm();
     let expr = r#" import! std.thread "#;
     let (value, _) = Compiler::new()
         .run_expr::<OpaqueValue<&Thread, Hole>>(&thread, "test", &expr)
         .unwrap_or_else(|err| panic!("{}", err));
     roundtrip(&thread, &value);
 }
+
+#[test]
+#[ignore] // Unimplemented so far
+fn roundtrip_thread() {
+    let thread = new_vm();
+    let expr = r#" let t = import! std.thread in t.new_thread ()"#;
+    let (value, _) = Compiler::new()
+        .run_io(true)
+        .run_expr::<IO<OpaqueValue<&Thread, Hole>>>(&thread, "test", &expr)
+        .unwrap_or_else(|err| panic!("{}", err));
+    roundtrip(&thread, &Into::<Result<_, _>>::into(value).unwrap());
+}

vm/Cargo.toml

@@ -30,6 +30,7 @@ ordered-float = "1"
 pretty = "0.5"
 quick-error = "1.1.0"
 smallvec = "0.6"
+slab = "0.4"
 typed-arena = "1.2.0"
 
 serde = { version = "1.0.0", optional = true }

vm/src/api/mod.rs

@@ -279,6 +279,15 @@ pub enum IO<T> {
     Exception(String),
 }
 
+impl<T> Into<StdResult<T, String>> for IO<T> {
+    fn into(self) -> StdResult<T, String> {
+        match self {
+            IO::Value(x) => Ok(x),
+            IO::Exception(x) => Err(x),
+        }
+    }
+}
+
 impl<T, E> From<StdResult<T, E>> for IO<T>
 where
     E: fmt::Display,

vm/src/serialization.rs

@@ -16,7 +16,7 @@ use crate::base::types::ArcType;
 
 use crate::array::Array;
 use crate::gc::{DataDef, GcPtr, WriteOnly};
-use crate::thread::{RootedThread, Thread, ThreadInternal};
+use crate::thread::{RootedThread, RootedValue, Thread, ThreadInternal};
 use crate::types::VmIndex;
 use crate::value::{
     BytecodeFunction, Callable, ClosureData, ExternFunction, PartialApplicationData,
@@ -114,6 +114,28 @@ pub mod gc {
     use crate::types::VmTag;
     use crate::value::{DataStruct, GcStr, ValueArray};
 
+    pub trait PostDeserialize {
+        fn init(ptr: GcPtr<Self>);
+    }
+
+    impl PostDeserialize for ExternFunction {
+        fn init(_: GcPtr<Self>) {}
+    }
+
+    impl PostDeserialize for BytecodeFunction {
+        fn init(_: GcPtr<Self>) {}
+    }
+
+    impl PostDeserialize for Thread {
+        fn init(mut ptr: GcPtr<Self>) {
+            // FIXME return an owned pointer from gc allocation so we don't need unsafe
+            unsafe {
+                let thread_index = ptr.parent_threads().insert((ptr, 0));
+                ptr.as_mut().thread_index = thread_index;
+            }
+        }
+    }
+
     impl Serialize for GcStr {
         fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
         where
@@ -125,7 +147,7 @@ pub mod gc {
 
     impl<'de, T> DeserializeState<'de, DeSeed> for crate::gc::Move<T>
     where
-        T: crate::gc::Trace,
+        T: crate::gc::Trace + PostDeserialize,
         T: DeserializeState<'de, DeSeed>,
     {
         fn deserialize_state<D>(seed: &mut DeSeed, deserializer: D) -> Result<Self, D::Error>
@@ -138,18 +160,20 @@ pub mod gc {
 
     impl<'de, T> DeserializeState<'de, DeSeed> for GcPtr<T>
     where
-        T: crate::gc::Trace + 'static,
+        T: crate::gc::Trace + PostDeserialize + 'static,
         T: DeserializeState<'de, DeSeed>,
     {
         fn deserialize_state<D>(seed: &mut DeSeed, deserializer: D) -> Result<Self, D::Error>
         where
             D: Deserializer<'de>,
         {
             use crate::gc::Move;
-            DeserializeSeed::deserialize(
+            let ptr = DeserializeSeed::deserialize(
                 Seed::<DataDefSeed<Move<T>>>::from(seed.clone()),
                 deserializer,
-            )
+            )?;
+
+            Ok(ptr)
         }
     }
 
@@ -788,6 +812,24 @@ impl<'a> crate::serde::ser::SerializeState<crate::serialization::SeSeed> for Var
     }
 }
 
+impl<'de> DeserializeState<'de, DeSeed> for RootedValue<RootedThread> {
+    fn deserialize_state<D>(seed: &mut DeSeed, deserializer: D) -> Result<Self, D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        eprintln!("before value ");
+        let value = Value::deserialize_state(seed, deserializer)?;
+        // TODO Prevent Value from being deserialized directly so we don't need to have this unsafe
+        eprintln!("after value ");
+        unsafe {
+            let x = Ok(seed.thread.root_value(Variants::new(&value)));
+
+            eprintln!("after value 2");
+            x
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     extern crate serde_json;