Skip to content

Commit

Permalink
Merge pull request #204 from Clikengo/fix_boot_api
Browse files Browse the repository at this point in the history 
Simplify and fix UB in boot API
  • Loading branch information
@Speedy37
Speedy37 authored Sep 17, 2020
2 parents cf5ceb6 + 4cee9d1 commit 7530fe5
Show file tree
Hide file tree
Showing 15 changed files with 229 additions and 228 deletions.
17 changes: 8 additions & 9 deletions foundationdb-bench/src/bin/fdb-bench.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
29 changes: 14 additions & 15 deletions foundationdb-bindingtester/src/main.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.");
}
39 changes: 38 additions & 1 deletion foundationdb/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
13 changes: 6 additions & 7 deletions foundationdb/examples/class-scheduling.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
129 changes: 61 additions & 68 deletions foundationdb/src/api.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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: (),
Expand All @@ -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()`
Expand Down Expand Up @@ -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)>,
}
Expand Down Expand Up @@ -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)>,
}
Expand All @@ -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: (),
}
Expand All @@ -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::*;
Expand Down
Loading

0 comments on commit 7530fe5

Please sign in to comment.