Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial code review #1

Merged
merged 6 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion lib/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ libloading = "0.8.3"
libc = "0.2.153"
rand = "0.8.5"
rustc_version_runtime = "0.3.0"
lazy_static = "1.4.0"
once_cell = "1"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lazy_static is effectively unmaintained and would be deprecated (rust-lang-nursery/lazy-static.rs#214). once_cell is the recommended migration path.
If we decide to only support rust 1.70+ (which is higher than our current internal toolchain version 1.69), we can implement the functionality from once_cell using OnceLock and cutoff the once_cell dependencies.
A more direct std substitute for once_cell::sync::Lazy is tracked in the unstable lazy_cell feature.

linkme = "0.3"
paste = "1.0.14"

Expand Down
5 changes: 2 additions & 3 deletions lib/src/assert/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use once_cell::sync::Lazy;
use serde_json::{Value, json};
use std::collections::HashMap;
use crate::internal;
Expand Down Expand Up @@ -221,9 +222,7 @@ impl AssertionInfo {
}
}

lazy_static!{
static ref ASSERT_TRACKER: Mutex<HashMap<String, TrackingInfo>> = Mutex::new(HashMap::new());
}
static ASSERT_TRACKER: Lazy<Mutex<HashMap<String, TrackingInfo>>> = Lazy::new(|| Mutex::new(HashMap::new()));

#[allow(clippy::too_many_arguments)]
pub fn assert_impl(
Expand Down
21 changes: 11 additions & 10 deletions lib/src/internal/local_handler.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,14 @@
use serde_json::{Value};
use std::env;
use std::fs::File;
use std::io::{Write, BufWriter, Error};
use std::io::{Write, Error};

use crate::internal::{LibHandler};

const LOCAL_OUTPUT: &str = "ANTITHESIS_SDK_LOCAL_OUTPUT";

// #[allow(dead_code)]
pub struct LocalHandler {
maybe_writer: Option<BufWriter<Box<dyn Write + Send>>>
maybe_writer: Option<File>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I hard coded the inner writer to File for now as it avoids a dynamic dispatch redirection introduced by Box<dyn Write>. Might have to revisit when we ever have to support dynamically switching the write sink for some reason?

}

impl LocalHandler {
Expand All @@ -27,7 +26,7 @@ impl LocalHandler {
// Seems like LocalHandler gets bound to a reference with
// a 'static lifetime.
LocalHandler{
maybe_writer: Some(BufWriter::with_capacity(0, Box::new(f)))
maybe_writer: Some(f)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the point of using a zero-capacity BufWriter. Since BufWriter is a wrapper over some underlying writer W, and writing to BufWriter only potentially writes (but not flush) to W, writing to a zero-capacity BufWriter should be equivalent to directly writing to W.

}
} else {
eprintln!("Unable to write to '{}' - {}", filename.as_str(), create_result.unwrap_err());
Expand All @@ -39,13 +38,15 @@ impl LocalHandler {
}

impl LibHandler for LocalHandler {
fn output(&mut self, value: &Value) -> Result<(), Error> {
let maybe_writer = self.maybe_writer.as_mut();
match maybe_writer {
fn output(&self, value: &Value) -> Result<(), Error> {
match &self.maybe_writer {
Some(b2w) => {
let mut text_line = value.to_string();
text_line.push('\n');
b2w.write_all(text_line.as_bytes())?;
let mut b2w = b2w;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider using a separate name for the mutable b2w, rather than shadowing the Some(b2w)

// The compact Display impl (selected using `{}`) of `serde_json::Value` contains no newlines,
// hence we are outputing valid JSONL format here.
// Using the `{:#}` format specifier may results in extra newlines and indentation.
// See https://docs.rs/serde_json/latest/serde_json/enum.Value.html#impl-Display-for-Value.
writeln!(b2w, "{}", value)?;
b2w.flush()?;
Ok(())
},
Expand Down
41 changes: 11 additions & 30 deletions lib/src/internal/mod.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
use once_cell::sync::Lazy;
use rustc_version_runtime::version;
use serde_json::{Value, json};
use std::io::{Error};
use std::sync::Mutex;
use local_handler::LocalHandler;
use voidstar_handler::{VoidstarHandler, has_voidstar};
use voidstar_handler::{VoidstarHandler};

mod local_handler;
mod voidstar_handler;
Expand All @@ -14,39 +14,21 @@ const PROTOCOL_VERSION: &str = "1.0.0";
// Tracks SDK releases
const SDK_VERSION: &str = "0.1.1";

// static mut LIB_HANDLER: Option<Box<dyn LibHandler>> = None;
static LIB_HANDLER: Mutex<Option<Box<dyn LibHandler + Send>>> = Mutex::new(None);
static LIB_HANDLER: Lazy<Box<dyn LibHandler + Sync + Send>> = Lazy::new(|| {
match VoidstarHandler::try_load() {
Ok(handler) => Box::new(handler),
Err(_) => Box::new(LocalHandler::new()),
}
});
Comment on lines -17 to +22
Copy link
Contributor Author

@wsx-antithesis wsx-antithesis Apr 23, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switch to using a Lazy cell for the global handler. Now the initialization logic for LIB_HANDLER would be called when it's content is access for the first time. So we can also remove manual calls to instantiate_handler() further down.
Lazy should have lower overhead compared to a Mutex, and we can remove the Mutex to guard against concurrent calls on LIB_HANDLER as we changed how LibHandler is implemented below.


trait LibHandler {
fn output(&mut self, value: &Value) -> Result<(), Error>;
fn output(&self, value: &Value) -> Result<(), Error>;
fn random(&self) -> u64;
}
Comment on lines 24 to 27
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ensure all LibHandler methods only takes &self. This has the benefit that all calls to those methods on LIB_HANDLER would be thread-safe without an extra Mutex.
The original user of the &mut self parameter is the implementation of LocalHandler::output, but we can avoid that since now LocalHandler stores an inner File, and &File implements Write already.


fn instantiate_handler() {
if LIB_HANDLER.lock().unwrap().is_some() {
return
}

let lh : Box<dyn LibHandler + Send> = if has_voidstar() {
Box::new(VoidstarHandler::new())
} else {
Box::new(LocalHandler::new())
};

{
let mut x = LIB_HANDLER.lock().unwrap();
*x = Some(lh);
}

let sdk_value: Value = sdk_info();
dispatch_output(&sdk_value)
}

// Made public so it can be invoked from the antithesis_sdk_rust::random module
pub fn dispatch_random() -> u64 {
instantiate_handler();
LIB_HANDLER.lock().unwrap().as_ref().unwrap().random()

LIB_HANDLER.random()
}

// Ignore any and all errors - either the output is completed,
Expand All @@ -66,8 +48,7 @@ pub fn dispatch_random() -> u64 {
// Made public so it can be invoked from the antithesis_sdk_rust::lifecycle
// and antithesis_sdk_rust::assert module
pub fn dispatch_output(json_data: &Value) {
instantiate_handler();
let _ = LIB_HANDLER.lock().unwrap().as_mut().unwrap().output(json_data);
let _ = LIB_HANDLER.output(json_data);
}

fn sdk_info() -> Value {
Expand Down
32 changes: 5 additions & 27 deletions lib/src/internal/voidstar_handler.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,47 +2,25 @@ use libc::{c_char, size_t};
use libloading::{Library, Symbol};
use serde_json::{Value};
use std::io::{Error};
use std::sync::{Once, Mutex, Arc};

use crate::internal::{LibHandler};

const LIB_NAME: &str = "/usr/lib/libmockstar.so";

pub fn has_voidstar() -> bool {
load_voidstar();
LIB_VOIDSTAR.lock().unwrap().is_some()
}

static LIB_VOIDSTAR: Mutex<Option<Arc<Library>>> = Mutex::new(None);

fn load_voidstar() {
static LOAD_VOIDSTAR: Once = Once::new();
LOAD_VOIDSTAR.call_once(|| {
let result = unsafe {
Library::new(LIB_NAME)
};
let mut lib_voidstar = LIB_VOIDSTAR.lock().unwrap();
*lib_voidstar = result.ok().map(Arc::new);
});
}

#[derive(Debug)]
pub struct VoidstarHandler {
voidstar_lib: Arc<Library>,
voidstar_lib: Library,
}

impl VoidstarHandler {
pub fn new() -> Self {
load_voidstar();
let lib = LIB_VOIDSTAR.lock().unwrap().as_ref().unwrap().clone();
VoidstarHandler{
voidstar_lib: lib,
}
pub fn try_load() -> Result<Self, libloading::Error> {
let lib = unsafe { Library::new(LIB_NAME) }?;
Ok(VoidstarHandler { voidstar_lib: lib })
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to avoid maintaining a local static for initializing the voidstar library here if the initialization logic at internal/mod.rs is enough?

}
}

impl LibHandler for VoidstarHandler {
fn output(&mut self, value: &Value) -> Result<(), Error> {
fn output(&self, value: &Value) -> Result<(), Error> {
let payload = value.to_string();
unsafe {
let json_data_func: Symbol<unsafe extern fn(s: *const c_char, l: size_t)> = self.voidstar_lib.get(b"fuzz_json_data").unwrap();
Expand Down
3 changes: 0 additions & 3 deletions lib/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
#[macro_use]
extern crate lazy_static;

pub mod assert;
pub mod lifecycle;
pub mod random;
Expand Down
Loading