Merge pull request #204 from Clikengo/fix_boot_api

Simplify and fix UB in boot API

Author: Speedy37

foundationdb-bench/src/bin/fdb-bench.rs

@@ -146,13 +146,12 @@ fn main() {
     let opt = Opt::from_args();
     info!("opt: {:?}", opt);
 
-    fdb::boot(|| {
-        let db = Arc::new(
-            futures::executor::block_on(fdb::Database::new_compat(None))
-                .expect("failed to get database"),
-        );
-
-        let bench = Bench { db, opt };
-        bench.run();
-    });
+    let _guard = unsafe { foundationdb::boot() };
+    let db = Arc::new(
+        futures::executor::block_on(fdb::Database::new_compat(None))
+            .expect("failed to get database"),
+    );
+
+    let bench = Bench { db, opt };
+    bench.run();
 }

foundationdb-bindingtester/src/main.rs

@@ -1582,23 +1582,22 @@ fn main() {
         "Starting rust bindingtester with api_version {}",
         api_version
     );
-    api::FdbApiBuilder::default()
+    let builder = api::FdbApiBuilder::default()
         .set_runtime_version(api_version)
         .build()
-        .expect("failed to initialize FoundationDB API")
-        .boot(|| {
-            let db = Arc::new(
-                futures::executor::block_on(fdb::Database::new_compat(cluster_path))
-                    .expect("failed to get database"),
-            );
-
-            let mut sm = StackMachine::new(&db, Bytes::from(prefix.to_owned().into_bytes()));
-            futures::executor::block_on(sm.run(db)).unwrap();
-            sm.join();
-
-            info!("Closing...");
-        })
-        .expect("failed to initialize FoundationDB network thread");
+        .expect("failed to initialize FoundationDB API");
+    let _network = unsafe { builder.boot() };
+
+    let db = Arc::new(
+        futures::executor::block_on(fdb::Database::new_compat(cluster_path))
+            .expect("failed to get database"),
+    );
+
+    let mut sm = StackMachine::new(&db, Bytes::from(prefix.to_owned().into_bytes()));
+    futures::executor::block_on(sm.run(db)).unwrap();
+    sm.join();
+
+    info!("Closing...");
 
     info!("Done.");
 }

foundationdb/README.md

@@ -65,11 +65,48 @@ async fn async_main() -> foundationdb::FdbResult<()> {
     Ok(())
 }
 
-foundationdb::boot(|| {
+foundationdb::run(|| {
     futures::executor::block_on(async_main()).expect("failed to run");
 });
 ```
 
+## Migration from 0.4 to 0.5
+
+The initialization of foundationdb API has changed due to undefined behavior being possible with only safe code (issues #170, #181, pulls #179, #182).
+
+Previously you had to wrote:
+
+```rust
+let network = foundationdb::boot().expect("failed to initialize Fdb");
+
+futures::executor::block_on(async_main()).expect("failed to run");
+// cleanly shutdown the client
+drop(network);
+```
+
+This can be converted to:
+
+```rust
+foundationdb::boot(|| {
+    futures::executor::block_on(async_main()).expect("failed to run");
+}).expect("failed to boot fdb");
+```
+
+or
+
+```rust
+#[tokio::main]
+async fn main() {
+    // Safe because drop is called before the program exits
+    let network = unsafe { foundationdb::boot() }.expect("failed to initialize Fdb");
+
+    // Have fun with the FDB API
+
+    // shutdown the client
+    drop(network);
+}
+```
+
 ## API stability
 
 _WARNING_ Until the 1.0 release of this library, the API may be in constant flux.

foundationdb/examples/class-scheduling.rs

@@ -364,11 +364,10 @@ async fn run_sim(db: &Database, students: usize, ops_per_student: usize) {
 }
 
 fn main() {
-    fdb::boot(|| {
-        let db = futures::executor::block_on(fdb::Database::new_compat(None))
-            .expect("failed to get database");
-        futures::executor::block_on(init(&db, &*ALL_CLASSES));
-        println!("Initialized");
-        futures::executor::block_on(run_sim(&db, 10, 10));
-    });
+    let _guard = unsafe { fdb::boot() };
+    let db = futures::executor::block_on(fdb::Database::new_compat(None))
+        .expect("failed to get database");
+    futures::executor::block_on(init(&db, &*ALL_CLASSES));
+    println!("Initialized");
+    futures::executor::block_on(run_sim(&db, 10, 10));
 }

foundationdb/src/api.rs

@@ -18,8 +18,6 @@ use std::sync::atomic::{AtomicBool, Ordering};
 use std::sync::{Arc, Condvar, Mutex};
 use std::thread;
 
-use futures::prelude::*;
-
 use crate::options::NetworkOption;
 use crate::{error, FdbResult};
 use foundationdb_sys as fdb_sys;
@@ -92,9 +90,9 @@ impl Default for FdbApiBuilder {
 /// use foundationdb::api::FdbApiBuilder;
 ///
 /// let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
-/// network_builder.boot(|| {
-///     // do some work with foundationDB
-/// }).expect("fdb network to run");
+/// let guard = unsafe { network_builder.boot() };
+/// // do some work with foundationDB
+/// drop(guard);
 /// ```
 pub struct NetworkBuilder {
     _private: (),
@@ -110,7 +108,7 @@ impl NetworkBuilder {
     /// Finalizes the initialization of the Network
     ///
     /// It's not recommended to use this method unless you really know what you are doing.
-    /// Otherwise, the `boot` method is the **safe** and easiest way to do it.
+    /// Otherwise, the `run` method is the **safe** and easiest way to do it.
     ///
     /// In order to start the network you have to call the unsafe `NetworkRunner::run()` method.
     /// This method starts the foundationdb network runloop, once started, the `NetworkStop::stop()`
@@ -144,90 +142,59 @@ impl NetworkBuilder {
         Ok((NetworkRunner { cond: cond.clone() }, NetworkWait { cond }))
     }
 
-    /// Execute `f` with the FoundationDB Client API ready, this can only be called once per process.
+    /// Initialize the FoundationDB Client API, this can only be called once per process.
+    ///
+    /// # Returns
+    ///
+    /// A `NetworkAutoStop` handle which must be dropped before the program exits.
+    ///
+    /// # Safety
+    ///
+    /// This method used to be safe in version `0.4`. But because `drop` on the returned object
+    /// might not be called before the program exits, it was found unsafe.
+    /// You should prefer the safe `run` variant.
+    /// If you still want to use this, you *MUST* ensure drop is called on the returned object
+    /// before the program exits. This is not required if the program is aborted.
     ///
     /// # Examples
     ///
     /// ```rust
     /// use foundationdb::api::FdbApiBuilder;
     ///
     /// let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
-    /// network_builder.boot(|| {
-    ///     // do some interesting things with the API...
-    /// });
+    /// let network = unsafe { network_builder.boot() };
+    /// // do some interesting things with the API...
+    /// drop(network);
     /// ```
-    pub fn boot<T>(self, f: impl (FnOnce() -> T) + panic::UnwindSafe) -> FdbResult<T> {
-        let (runner, cond) = self.build()?;
-
-        let net_thread = thread::spawn(move || {
-            unsafe { runner.run() }.expect("failed to run");
-        });
-
-        let network = cond.wait();
-
-        let res = panic::catch_unwind(f);
-
-        if let Err(err) = network.stop() {
-            eprintln!("failed to stop network: {}", err);
-            // Not aborting can probably cause undefined behavior
-            std::process::abort();
-        }
-        net_thread.join().expect("failed to join fdb thread");
-
-        match res {
-            Err(payload) => panic::resume_unwind(payload),
-            Ok(v) => Ok(v),
-        }
-    }
-
-    /// Async execute `f` with the FoundationDB Client API ready, this can only be called once per process.
-    ///
-    /// # Examples
     ///
     /// ```rust
     /// use foundationdb::api::FdbApiBuilder;
     ///
-    /// let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
-    /// network_builder.boot_async(|| async {
+    /// #[tokio::main]
+    /// async fn main() {
+    ///     let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
+    ///     let network = unsafe { network_builder.boot() };
     ///     // do some interesting things with the API...
-    /// });
+    ///     drop(network);
+    /// }
     /// ```
-    pub async fn boot_async<F, Fut, T>(self, f: F) -> FdbResult<T>
-    where
-        F: (FnOnce() -> Fut) + panic::UnwindSafe,
-        Fut: Future<Output = T> + panic::UnwindSafe,
-    {
+    pub unsafe fn boot(self) -> FdbResult<NetworkAutoStop> {
         let (runner, cond) = self.build()?;
 
-        let net_thread = thread::spawn(move || {
-            unsafe { runner.run() }.expect("failed to run");
-        });
+        let net_thread = runner.spawn();
 
         let network = cond.wait();
 
-        let res = panic::catch_unwind(f);
-        let res = match res {
-            Ok(fut) => fut.catch_unwind().await,
-            Err(err) => Err(err),
-        };
-
-        if let Err(err) = network.stop() {
-            eprintln!("failed to stop network: {}", err);
-            // Not aborting can probably cause undefined behavior
-            std::process::abort();
-        }
-        net_thread.join().expect("failed to join fdb thread");
-
-        match res {
-            Err(payload) => panic::resume_unwind(payload),
-            Ok(v) => Ok(v),
-        }
+        Ok(NetworkAutoStop {
+            handle: Some(net_thread),
+            network: Some(network),
+        })
     }
 }
 
 /// A foundationDB network event loop runner
 ///
-/// Most of the time you should never need to use this directly and use `NetworkBuilder::boot()`.
+/// Most of the time you should never need to use this directly and use `NetworkBuilder::run()`.
 pub struct NetworkRunner {
     cond: Arc<(Mutex<bool>, Condvar)>,
 }
@@ -257,11 +224,17 @@ impl NetworkRunner {
 
         error::eval(unsafe { fdb_sys::fdb_run_network() })
     }
+
+    unsafe fn spawn(self) -> thread::JoinHandle<()> {
+        thread::spawn(move || {
+            self.run().expect("failed to run network thread");
+        })
+    }
 }
 
 /// A condition object that can wait for the associated `NetworkRunner` to actually run.
 ///
-/// Most of the time you should never need to use this directly and use `NetworkBuilder::boot()`.
+/// Most of the time you should never need to use this directly and use `NetworkBuilder::run()`.
 pub struct NetworkWait {
     cond: Arc<(Mutex<bool>, Condvar)>,
 }
@@ -284,7 +257,7 @@ impl NetworkWait {
 
 /// Allow to stop the associated and running `NetworkRunner`.
 ///
-/// Most of the time you should never need to use this directly and use `NetworkBuilder::boot()`.
+/// Most of the time you should never need to use this directly and use `NetworkBuilder::run()`.
 pub struct NetworkStop {
     _private: (),
 }
@@ -296,6 +269,26 @@ impl NetworkStop {
     }
 }
 
+/// Stop the associated `NetworkRunner` and thread if dropped
+pub struct NetworkAutoStop {
+    network: Option<NetworkStop>,
+    handle: Option<std::thread::JoinHandle<()>>,
+}
+impl Drop for NetworkAutoStop {
+    fn drop(&mut self) {
+        if let Err(err) = self.network.take().unwrap().stop() {
+            eprintln!("failed to stop network: {}", err);
+            // Not aborting can probably cause undefined behavior
+            std::process::abort();
+        }
+        self.handle
+            .take()
+            .unwrap()
+            .join()
+            .expect("failed to join fdb thread");
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;