Simplify and fix UB in boot API

- remove UB `boot_async` method
- rename `boot`  to `run` method
- add back the old but now unsafe `boot` method
- add migration guide from 0.4 to 0.5

#202

Author: Vincent Rouillé

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

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

foundationdb-bindingtester/src/main.rs

@@ -1586,7 +1586,7 @@ fn main() {
         .set_runtime_version(api_version)
         .build()
         .expect("failed to initialize FoundationDB API")
-        .boot(|| {
+        .run(|| {
             let db = Arc::new(
                 futures::executor::block_on(fdb::Database::new_compat(cluster_path))
                     .expect("failed to get database"),

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,7 +364,7 @@ async fn run_sim(db: &Database, students: usize, ops_per_student: usize) {
 }
 
 fn main() {
-    fdb::boot(|| {
+    fdb::run(|| {
         let db = futures::executor::block_on(fdb::Database::new_compat(None))
             .expect("failed to get database");
         futures::executor::block_on(init(&db, &*ALL_CLASSES));

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,7 +90,7 @@ impl Default for FdbApiBuilder {
 /// use foundationdb::api::FdbApiBuilder;
 ///
 /// let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
-/// network_builder.boot(|| {
+/// network_builder.run(|| {
 ///     // do some work with foundationDB
 /// }).expect("fdb network to run");
 /// ```
@@ -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()`
@@ -145,89 +143,82 @@ impl NetworkBuilder {
     }
 
     /// Execute `f` with the FoundationDB Client API ready, this can only be called once per process.
+    /// Once this method returns, the FoundationDB Client API is stopped and cannot be restarted.
     ///
     /// # Examples
     ///
     /// ```rust
     /// use foundationdb::api::FdbApiBuilder;
     ///
     /// let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
-    /// network_builder.boot(|| {
+    /// network_builder.run(|| {
     ///     // do some interesting things with the API...
     /// });
     /// ```
-    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();
+    pub fn run<T>(self, f: impl FnOnce() -> T) -> FdbResult<T> {
+        // Safe because the returned object is dropped before this function returns
+        let must_drop = unsafe { self.boot()? };
 
-        let res = panic::catch_unwind(f);
+        let t = 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");
+        drop(must_drop);
 
-        match res {
-            Err(payload) => panic::resume_unwind(payload),
-            Ok(v) => Ok(v),
-        }
+        Ok(t)
     }
 
-    /// Async 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_async(|| async {
+    /// let network = unsafe { network_builder.boot() }.expect("fdb network running");
+    /// // do some interesting things with the API...
+    /// drop(network);
+    /// ```
+    ///
+    /// ```rust
+    /// use foundationdb::api::FdbApiBuilder;
+    ///
+    /// #[tokio::main]
+    /// async fn main() {
+    ///     let network_builder = FdbApiBuilder::default().build().expect("fdb api initialized");
+    ///     let network = unsafe { network_builder.boot() }.expect("fdb network running");
     ///     // 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 +248,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 +281,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 +293,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::*;

foundationdb/src/lib.rs

@@ -70,7 +70,7 @@
 //!     Ok(())
 //! }
 //!
-//! foundationdb::boot(|| {
+//! foundationdb::run(|| {
 //!     futures::executor::block_on(async_main()).expect("failed to run");
 //! });
 //! ```
@@ -109,38 +109,53 @@ pub use crate::transaction::*;
 /// # Examples
 ///
 /// ```rust
-/// foundationdb::boot(|| {
+/// foundationdb::run(|| {
 ///     // do some interesting things with the API...
 /// });
 /// ```
-pub fn boot<T>(f: impl (FnOnce() -> T) + std::panic::UnwindSafe) -> T {
+pub fn run<T>(f: impl FnOnce() -> T) -> T {
     api::FdbApiBuilder::default()
         .build()
         .expect("foundationdb API to be initialized")
-        .boot(f)
+        .run(f)
         .expect("foundationdb network to be setup")
 }
 
-/// Async 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
-/// foundationdb::boot_async(|| async {
+/// let network = unsafe { foundationdb::boot() };
+/// // do some interesting things with the API...
+/// drop(network);
+/// ```
+///
+/// ```rust
+/// #[tokio::main]
+/// async fn main() {
+///     let network = unsafe { foundationdb::boot() };
 ///     // do some interesting things with the API...
-/// });
+///     drop(network);
+/// }
 /// ```
-pub async fn boot_async<F, Fut, T>(f: F) -> T
-where
-    F: (FnOnce() -> Fut) + std::panic::UnwindSafe,
-    Fut: std::future::Future<Output = T> + std::panic::UnwindSafe,
-{
-    api::FdbApiBuilder::default()
+pub unsafe fn boot() -> api::NetworkAutoStop {
+    let network_builder = api::FdbApiBuilder::default()
         .build()
-        .expect("foundationdb API to be initialized")
-        .boot_async(f)
-        .await
-        .expect("foundationdb network to be setup")
+        .expect("foundationdb API to be initialized");
+    network_builder.boot().expect("fdb network running")
 }
 
 /// Returns the default Fdb cluster configuration file path

foundationdb/tests/atomic.rs

@@ -12,7 +12,7 @@ mod common;
 
 #[test]
 fn test_atomic() {
-    boot(|| futures::executor::block_on(test_atomic_async()).expect("failed to run"));
+    run(|| futures::executor::block_on(test_atomic_async()).expect("failed to run"));
 }
 
 async fn atomic_add(db: &Database, key: &[u8], value: i64) -> FdbResult<()> {

foundationdb/tests/future.rs

@@ -32,7 +32,7 @@ where
 
 #[test]
 fn test_future_discard() {
-    boot(|| futures::executor::block_on(test_future_discard_async()).expect("failed to run"));
+    run(|| futures::executor::block_on(test_future_discard_async()).expect("failed to run"));
 }
 
 async fn test_future_discard_async() -> FdbResult<()> {

foundationdb/tests/get.rs

@@ -13,7 +13,7 @@ mod common;
 
 #[test]
 fn test_get() {
-    boot(|| {
+    run(|| {
         futures::executor::block_on(test_set_get_async()).expect("failed to run");
         futures::executor::block_on(test_get_multi_async()).expect("failed to run");
         futures::executor::block_on(test_set_conflict_async()).expect("failed to run");