Merge branch 'rpclib-safer-handle' into 'main' (5e2d5532) · Commits · NoisyCoil / Arti

crates/arti-rpc-client-core/arti-rpc-client-core.h

+9 −5

Original line number	Diff line number	Diff line
		@@ -91,6 +91,11 @@
		* - If a function consumes (takes ownership of) one of its inputs,
		* it does so regardless of whether the function succeeds or fails.
		*
		* - Whenever one or more functions take an argument via a `const Type *`,
		* it is safe to pass the same object to multiple functions at once.
		* (This does not apply to functions that take an argument via a
		* non-const pointer.)
		*
		* ## Correctness requirements
		*
		* If any correctness requirements stated here or elsewhere are violated,
		@@ -410,10 +415,9 @@ ArtiRpcStatus arti_rpc_conn_execute_with_handle(const ArtiRpcConn *rpc_conn,
		* and deliver the error from Arti in `response_out`, setting `response_type_out` to
		* `ARTI_RPC_RESPONSE_TYPE_ERROR`.
		*
		* # Correctness requirements
		*
		* No other thread or code must be using `handle` while this function is running.
		* Accessing `handle` from multiple functions at once may result in undefined behavior.
		* It is safe to call this function on the same handle from multiple threads at once.
		* If you do, each response will be sent to exactly one thread.
		* It is unspecified which thread will receive which response or which error.
		*
		* # Ownership
		*
		@@ -421,7 +425,7 @@ ArtiRpcStatus arti_rpc_conn_execute_with_handle(const ArtiRpcConn *rpc_conn,
		*
		* The caller is responsible for making sure that `*response_out`, if set, is eventually freed.
		*/
		ArtiRpcStatus arti_rpc_handle_wait(ArtiRpcHandle *handle,
		ArtiRpcStatus arti_rpc_handle_wait(const ArtiRpcHandle *handle,
		ArtiRpcStr **response_out,
		ArtiRpcResponseType *response_type_out,
		ArtiRpcError **error_out);

crates/arti-rpc-client-core/cbindgen.toml

+6 −7

Original line number	Diff line number	Diff line
		@@ -100,6 +100,11 @@ header = """\
		* - If a function consumes (takes ownership of) one of its inputs,
		* it does so regardless of whether the function succeeds or fails.
		*
		* - Whenever one or more functions take an argument via a `const Type *`,
		* it is safe to pass the same object to multiple functions at once.
		* (This does not apply to functions that take an argument via a
		* non-const pointer.)
		*
		* ## Correctness requirements
		*
		* If any correctness requirements stated here or elsewhere are violated,
		@@ -151,13 +156,7 @@ tab_width = 8
		[export]
		# These structs are not ones we want to expose under their actual names,
		# or ones that we don't want to expose at all.
		exclude = [
		"FfiError",
		"RequestHandle",
		"RpcConn",
		"RpcErrorCode",
		"Utf8CString"
		]
		exclude = ["FfiError", "RequestHandle", "RpcConn", "RpcErrorCode", "Utf8CString"]

		[export.rename]
		# Having not declared these structs, we can give them new names in the

crates/arti-rpc-client-core/src/conn.rs

+18 −6

Original line number	Diff line number	Diff line
		@@ -6,7 +6,7 @@
		use std::{
		io::{self, BufReader},
		path::PathBuf,
		sync::Arc,
		sync::{Arc, Mutex},
		};

		use crate::{
		@@ -32,8 +32,16 @@ pub use connimpl::RpcConn;
		#[educe(Debug)]
		pub struct RequestHandle {
		/// The underlying `Receiver` that we'll use to get updates for this request
		///
		/// It's wrapped in a `Mutex` to prevent concurrent calls to `Receiver::wait_on_message_for`.
		//
		// NOTE: As an alternative to using a Mutex here, we _could_ remove
		// the restriction from `wait_on_message_for` that says that only one thread
		// may be waiting on a given request ID at once. But that would introduce
		// complexity to the implementation,
		// and it's not clear that the benefit would be worth it.
		#[educe(Debug(ignore))]
		conn: Arc<connimpl::Receiver>,
		conn: Mutex<Arc<connimpl::Receiver>>,
		/// The ID of this request.
		id: AnyRequestId,
		}
		@@ -265,7 +273,7 @@ impl RpcConn {
		where
		F: FnMut(UpdateResponse) + Send + Sync,
		{
		let mut hnd = self.execute_with_handle(cmd)?;
		let hnd = self.execute_with_handle(cmd)?;
		loop {
		match hnd.wait_with_updates()? {
		AnyResponse::Success(s) => return Ok(Ok(s)),
		@@ -290,7 +298,7 @@ impl RequestHandle {
		/// Note that this function will return `Err(.)` only if sending the command or getting a
		/// response failed. If the command was sent successfully, and Arti reported an error in response,
		/// this function returns `Ok(Err(.))`.
		pub fn wait(mut self) -> Result<FinalResponse, ProtoError> {
		pub fn wait(self) -> Result<FinalResponse, ProtoError> {
		loop {
		match self.wait_with_updates()? {
		AnyResponse::Success(s) => return Ok(Ok(s)),
		@@ -305,11 +313,15 @@ impl RequestHandle {
		/// response failed. If the command was sent successfully, and Arti reported an error in response,
		/// this function returns `Ok(AnyResponse::Error(.))`.
		///
		/// You may call this method on the same `RequestHandle` from multiple threads.
		/// If you do so, those calls will receive responses (or errors) in an unspecified order.
		///
		/// If this function returns Success or Error, then you shouldn't call it again.
		/// All future calls to this function will fail with `CmdError::RequestCancelled`.
		/// (TODO RPC: Maybe rename that error.)
		pub fn wait_with_updates(&mut self) -> Result<AnyResponse, ProtoError> {
		let validated = self.conn.wait_on_message_for(&self.id)?;
		pub fn wait_with_updates(&self) -> Result<AnyResponse, ProtoError> {
		let conn = self.conn.lock().expect("Poisoned lock");
		let validated = conn.wait_on_message_for(&self.id)?;

		Ok(AnyResponse::from_validated(validated))
		}

crates/arti-rpc-client-core/src/conn/connimpl.rs

+1 −1

Original line number	Diff line number	Diff line
		@@ -253,7 +253,7 @@ impl RpcConn {

		Ok(()) => Ok(super::RequestHandle {
		id,
		conn: Arc::clone(&self.receiver),
		conn: Mutex::new(Arc::clone(&self.receiver)),
		}),
		}
		}

crates/arti-rpc-client-core/src/ffi.rs

+5 −6

Original line number	Diff line number	Diff line
		@@ -246,10 +246,9 @@ impl AnyResponse {
		/// and deliver the error from Arti in `response_out`, setting `response_type_out` to
		/// `ARTI_RPC_RESPONSE_TYPE_ERROR`.
		///
		/// # Correctness requirements
		///
		/// No other thread or code must be using `handle` while this function is running.
		/// Accessing `handle` from multiple functions at once may result in undefined behavior.
		/// It is safe to call this function on the same handle from multiple threads at once.
		/// If you do, each response will be sent to exactly one thread.
		/// It is unspecified which thread will receive which response or which error.
		///
		/// # Ownership
		///
		@@ -259,14 +258,14 @@ impl AnyResponse {
		#[allow(clippy::missing_safety_doc)]
		#[no_mangle]
		pub unsafe extern "C" fn arti_rpc_handle_wait(
		handle: *mut ArtiRpcHandle,
		handle: *const ArtiRpcHandle,
		response_out: mut mut ArtiRpcStr,
		response_type_out: *mut ArtiRpcResponseType,
		error_out: mut mut ArtiRpcError,
		) -> ArtiRpcStatus {
		ffi_body_with_err! {
		{
		let handle: Option<&mut ArtiRpcHandle> [in_mut_ptr_opt];
		let handle: Option<&ArtiRpcHandle> [in_ptr_opt];
		let response_out: Option<OutPtr<ArtiRpcStr>> [out_ptr_opt];
		let response_type_out: Option<OutVal<ArtiRpcResponseType>> [out_val_opt];
		err error_out: Option<OutPtr<ArtiRpcError>>;