From e26d9452dcd340146bb4bf5853632ded18c71aef Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Mon, 3 Apr 2023 10:23:55 -0400 Subject: [PATCH] RPC: Initial implementation of a multiple-argument dispatch This code uses some kludges (discussed with Ian previously and hopefully well documented here) to get a type-identifier for each type in a const context. It then defines a macro to declare a type-erased versions of a concrete implementation functions, and register those implementations to be called later. We will probably want to tweak a bunch of this code as we move ahead. --- Cargo.lock | 11 ++ crates/arti-rpcserver/src/msgs.rs | 1 + crates/tor-rpccmd/Cargo.toml | 10 +- crates/tor-rpccmd/src/cmd.rs | 11 +- crates/tor-rpccmd/src/dispatch.rs | 228 ++++++++++++++++++++++++++++++ crates/tor-rpccmd/src/lib.rs | 54 +++++++ crates/tor-rpccmd/src/obj.rs | 6 +- crates/tor-rpccmd/src/typeid.rs | 99 +++++++++++++ 8 files changed, 412 insertions(+), 8 deletions(-) create mode 100644 crates/tor-rpccmd/src/dispatch.rs create mode 100644 crates/tor-rpccmd/src/typeid.rs diff --git a/Cargo.lock b/Cargo.lock index 27602d822..bc0ccc493 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -270,6 +270,12 @@ dependencies = [ "tracing-subscriber", ] +[[package]] +name = "assert-impl" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c3464313de0c867016e3e69d7e1e9ae3499bcc4c18e12283d381359ed38b5b9e" + [[package]] name = "async-broadcast" version = "0.5.1" @@ -4379,8 +4385,13 @@ dependencies = [ name = "tor-rpccmd" version = "0.0.1" dependencies = [ + "assert-impl", + "downcast-rs", "futures", "futures-await-test", + "inventory", + "once_cell", + "paste", "serde", "thiserror", "typetag", diff --git a/crates/arti-rpcserver/src/msgs.rs b/crates/arti-rpcserver/src/msgs.rs index a626be016..1297c4b3c 100644 --- a/crates/arti-rpcserver/src/msgs.rs +++ b/crates/arti-rpcserver/src/msgs.rs @@ -148,6 +148,7 @@ mod test { #[typetag::deserialize(name = "dummy")] impl rpc::Command for DummyCmd {} + tor_rpccmd::impl_const_type_id!(DummyCmd); // XXXX #[derive(Serialize)] struct DummyResponse { diff --git a/crates/tor-rpccmd/Cargo.toml b/crates/tor-rpccmd/Cargo.toml index be103df92..0b0819b67 100644 --- a/crates/tor-rpccmd/Cargo.toml +++ b/crates/tor-rpccmd/Cargo.toml @@ -12,12 +12,16 @@ categories = ["asynchronous"] repository = "https://gitlab.torproject.org/tpo/core/arti.git/" [dependencies] -#XXXX remove whatever is unneeded here. +downcast-rs = "1.2.0" futures = "0.3.14" -serde = { version = "1.0.103", features = [ "derive"] } +futures-await-test = "0.3.0" +inventory = "0.3.5" +once_cell = "1" +paste = "1" +serde = { version = "1.0.103", features = ["derive"] } thiserror = "1" typetag = "0.2.7" [dev-dependencies] -#XXXX remove whatever is unneeded here. +assert-impl = "0.1.3" futures-await-test = "0.3.0" diff --git a/crates/tor-rpccmd/src/cmd.rs b/crates/tor-rpccmd/src/cmd.rs index 0730eab80..25bf09275 100644 --- a/crates/tor-rpccmd/src/cmd.rs +++ b/crates/tor-rpccmd/src/cmd.rs @@ -1,3 +1,7 @@ +use downcast_rs::Downcast; + +use crate::typeid::GetConstTypeId_; + /// The parameters and method name associated with a given Request. /// /// We use [`typetag`] here so that we define `Command`s in other crates. @@ -10,7 +14,6 @@ // TODO RPC: Possible issue here is that, if this trait is public, anybody outside // of Arti can use this trait to add new commands to the RPC engine. Should we // care? -#[typetag::deserialize(tag = "method", content = "data")] -pub trait Command: std::fmt::Debug + Send { - // TODO RPC: this will need some kind of "run this command" trait. -} +#[typetag::deserialize(tag = "method", content = "params")] +pub trait Command: GetConstTypeId_ + std::fmt::Debug + Send + Downcast {} +downcast_rs::impl_downcast!(Command); diff --git a/crates/tor-rpccmd/src/dispatch.rs b/crates/tor-rpccmd/src/dispatch.rs new file mode 100644 index 000000000..a843dadbe --- /dev/null +++ b/crates/tor-rpccmd/src/dispatch.rs @@ -0,0 +1,228 @@ +//! A multiple-argument dispatch system for our RPC system. +//! +//! Our RPC functionality is polymorphic in Commands (what we're told to do) and +//! Objects (the things that we give the commands to); we want to be able to +//! provide different implementations for each command, on each object. + +use std::collections::HashMap; +use std::sync::Arc; + +use futures::future::BoxFuture; +use once_cell::sync::Lazy; + +use crate::typeid::ConstTypeId_; +use crate::{Command, Context, Object}; + +/// The return type from an RPC function. +type RpcResult = String; // XXXX Not the actual type we want. + +// A boxed future holding the result of an RPC command. +type RpcResultFuture = BoxFuture<'static, RpcResult>; + +/// A type-erased RPC-command invocation function. +/// +/// This function takes `Arc`s rather than a reference, so that it can return a +/// `'static` future. +type ErasedInvokeFn = fn(Arc, Box, Arc) -> RpcResultFuture; + +/// An entry for our dynamic dispatch code. +/// +/// These are generated using [`inventory`] by our `rpc_invoke_fn` macro; +/// they are later collected into a more efficient data structure. +#[doc(hidden)] +pub struct InvokeEntry_ { + obj_id: ConstTypeId_, + cmd_id: ConstTypeId_, + func: ErasedInvokeFn, +} + +// Note that using `inventory` here means that _anybody_ can define new +// commands! This may not be the greatest property. +inventory::collect!(InvokeEntry_); + +impl InvokeEntry_ { + /// Create a new `InvokeEntry_`. + #[doc(hidden)] + pub const fn new(obj_id: ConstTypeId_, cmd_id: ConstTypeId_, func: ErasedInvokeFn) -> Self { + InvokeEntry_ { + obj_id, + cmd_id, + func, + } + } +} + +/// Declare an RPC function that will be used to call a single type of [`Command`] on a +/// single type of [`Object`]. +/// +/// # Example +/// +/// ``` +/// use tor_rpccmd as rpc; +/// +/// #[derive(Debug)] +/// struct ExampleObject {} +/// impl rpc::Object for ExampleObject {} +/// +/// #[derive(Debug,serde::Deserialize)] +/// struct ExampleCommand {} +/// #[typetag::deserialize] +/// impl rpc::Command for ExampleCommand {} +/// +/// // TODO RPC Hide this macro. +/// rpc::impl_const_type_id!{ExampleObject ExampleCommand} +/// +/// rpc::rpc_invoke_fn!{ +/// // XXXX wrong return type. +/// async fn example(obj: ExampleObject, cmd: ExampleCommand, ctx) -> String { +/// // XXXX In this function, obj is actually an Arc, +/// // cmd is actually a Box, +/// // and ctx is actually an Arc! +/// // +/// // XXXX This is quite infelicitous, since the details are hidden. We should +/// // probably make the type information explicit instead. +/// println!("Running example command!"); +/// "here is your result".into() +/// } +/// } +/// ``` +#[macro_export] +macro_rules! rpc_invoke_fn { + { + $(#[$meta:meta])* + $v:vis async fn $name:ident($obj:ident : $objtype:ty, $cmd:ident: $cmdtype:ty, $ctx:ident) -> $rtype:ty { + $($body:tt)* + } + $( $($more:tt)+ )? + } => {$crate::paste::paste!{ + // First we declare the function that the user gave us. + $(#[$meta])* + // XXXX mangling the types here is not good; see comment in example above. + $v async fn $name($obj: std::sync::Arc<$objtype>, $cmd: Box<$cmdtype>, $ctx: std::sync::Arc) -> $rtype { + $($body)* + } + // Now we declare a type-erased version of the function that takes Arc and Box arguments, and returns + // a boxed future. + #[doc(hidden)] + fn [<_typeerased_ $name>](obj: std::sync::Arc, cmd: Box, ctx: std::sync::Arc) -> $crate::futures::future::BoxFuture<'static, $rtype> { + use $crate::futures::FutureExt; + let obj = obj + .downcast_arc::<$objtype>() + .unwrap_or_else(|_| panic!()); + let cmd = cmd + .downcast::<$cmdtype>() + .unwrap_or_else(|_| panic!()); + $name(obj, cmd, ctx).boxed() + } + // Finally we use `inventory` to register the type-erased function with + // the right types. + $crate::inventory::submit!{ + $crate::dispatch::InvokeEntry_::new( + <$objtype as $crate::typeid::HasConstTypeId_>::CONST_TYPE_ID_, + <$cmdtype as $crate::typeid::HasConstTypeId_>::CONST_TYPE_ID_, + [<_typeerased_ $name >] + ) + } + + $(rpc_invoke_fn!{$($more)+})? + }} +} + +/// Actual types to use when looking up a function in our HashMap. +#[derive(Eq, PartialEq, Clone, Debug, Hash)] +struct FuncType { + obj_id: ConstTypeId_, + cmd_id: ConstTypeId_, +} + +/// Table mapping `FuncType` to `ErasedInvokeFn`. +/// +/// This is constructed once, the first time we use our dispatch code. +static FUNCTION_TABLE: Lazy> = Lazy::new(|| { + // We want to assert that there are no duplicates, so we can't use "collect" + let mut map = HashMap::new(); + for ent in inventory::iter::() { + let InvokeEntry_ { + obj_id, + cmd_id, + func, + } = *ent; + let old_val = map.insert(FuncType { obj_id, cmd_id }, func); + assert!( + old_val.is_none(), + "Tried to register two RPC functions with the same type IDs!" + ); + } + map +}); + +/// An error that occurred while trying to invoke a command on an object. +#[derive(Debug, thiserror::Error)] +#[non_exhaustive] +pub enum InvokeError { + /// There is no implementation for the given combination of object + /// type and command type. + #[error("No implementation for provided object and command types.")] + NoImpl, +} + +/// Try to find an appropriate function for calling a given RPC command on a +/// given RPC-visible object. +/// +/// On success, return a Future. +pub fn invoke_command( + obj: Arc, + cmd: Box, + ctx: Arc, +) -> Result { + let func_type = FuncType { + obj_id: obj.const_type_id(), + cmd_id: cmd.const_type_id(), + }; + + let func = FUNCTION_TABLE.get(&func_type).ok_or(InvokeError::NoImpl)?; + + Ok(func(obj, cmd, ctx)) +} + +#[cfg(test)] +mod test { + use futures_await_test::async_test; + + pub struct Animal {} + + #[derive(Debug, serde::Deserialize)] + pub struct SayHi {} + crate::impl_const_type_id!(Animal SayHi); + impl crate::Object for Animal {} + #[typetag::deserialize] + impl crate::Command for SayHi {} + + rpc_invoke_fn! { + /// Hello there + async fn invoke(_obj: Animal, cmd: SayHi, _ctx) -> String { + format!("{:?}", cmd) + } + } + + struct Ctx {} + impl crate::Context for Ctx { + fn lookup_object( + &self, + _id: &crate::ObjectId, + ) -> Option> { + todo!() + } + } + + // TODO RPC: Improve this test! + #[async_test] + async fn t() { + use super::*; + let animal: Arc = Arc::new(Animal {}); + let hi: Box = Box::new(SayHi {}); + let ctx = Arc::new(Ctx {}); + let s = invoke_command(animal, hi, ctx).unwrap().await; + assert_eq!(&s, "SayHi"); + } +} diff --git a/crates/tor-rpccmd/src/lib.rs b/crates/tor-rpccmd/src/lib.rs index 33521ad8f..54b7a67db 100644 --- a/crates/tor-rpccmd/src/lib.rs +++ b/crates/tor-rpccmd/src/lib.rs @@ -4,7 +4,61 @@ //! mod cmd; +pub mod dispatch; mod obj; +#[doc(hidden)] +pub mod typeid; + +use std::sync::Arc; pub use cmd::Command; +pub use dispatch::invoke_command; pub use obj::{Object, ObjectId}; + +#[doc(hidden)] +pub use {downcast_rs, futures, inventory, paste}; + +/// An error returned from [`ContextExt::lookup`]. +/// +/// TODO RPC: This type should be made to conform with however we represent RPC +/// errors. +#[derive(Debug, Clone, thiserror::Error)] +#[non_exhaustive] +pub enum LookupError { + /// The specified object does not (currently) exist, + /// or the user does not have permission to access it. + #[error("No visible object with ID {0:?}")] + NoObject(ObjectId), + + /// The specified object exists, but does not have the + /// expected type. + #[error("Unexpected type on object with ID {0:?}")] + WrongType(ObjectId), +} + +/// A trait describing the context in which an RPC command is executed. +pub trait Context: Send + Sync { + /// Look up an object by identity within this context. + /// + /// A return of `None` may indicate that the object has disappeared, + /// that the object doesn't exist, + /// that the [`ObjectId`] is ill-formed, + /// or that the user has no permission to access the object. + fn lookup_object(&self, id: &ObjectId) -> Option>; +} + +/// Extension trait for [`Context`]. +/// +/// This is a separate trait so that `Context` can be object-safe. +pub trait ContextExt: Context { + /// Look up an object of a given type, and downcast it. + /// + /// Return an error if the object can't be found, or has the wrong type. + fn lookup(&self, id: &ObjectId) -> Result, LookupError> { + self.lookup_object(id) + .ok_or_else(|| LookupError::NoObject(id.clone()))? + .downcast_arc() + .map_err(|_| LookupError::WrongType(id.clone())) + } +} +impl ContextExt for T {} diff --git a/crates/tor-rpccmd/src/obj.rs b/crates/tor-rpccmd/src/obj.rs index c18bce5d6..cad908f1f 100644 --- a/crates/tor-rpccmd/src/obj.rs +++ b/crates/tor-rpccmd/src/obj.rs @@ -1,6 +1,10 @@ +use downcast_rs::DowncastSync; use serde::{Deserialize, Serialize}; -pub trait Object {} +use crate::typeid::GetConstTypeId_; + +pub trait Object: GetConstTypeId_ + DowncastSync {} +downcast_rs::impl_downcast!(sync Object); /// An identifier for an Object within the context of a Session. /// diff --git a/crates/tor-rpccmd/src/typeid.rs b/crates/tor-rpccmd/src/typeid.rs new file mode 100644 index 000000000..6050f0f38 --- /dev/null +++ b/crates/tor-rpccmd/src/typeid.rs @@ -0,0 +1,99 @@ +//! A kludgy replacement for [`std::any::TypeId`] that can be used in a constant context. + +/// A less helpful variant of `std::any::TypeId` that can be used in a const +/// context. +/// +/// Until the [relevant Rust feature] is stabilized, it's not possible to get a +/// TypeId for a type and store it in a const. But sadly, we need to do so for +/// our dispatch code. +/// +/// Thus, we use a nasty hack: we use the address of the function +/// `TypeId::of::` as the identifier for the type of T. +/// +/// This type and the module containing it are hidden: Nobody should actually +/// use it outside of our dispatch code. Once we can use `TypeId` instead, we +/// should and will. +/// +/// To make a type participate in this system, use the [`impl_const_type_id`] +/// macro. +/// +/// **Do not mention this type outside of this module.** +/// +/// [relevant Rust feature]: https://github.com/rust-lang/rust/issues/77125 +#[derive(Debug, Eq, PartialEq, Hash, Clone, Copy)] +pub struct ConstTypeId_( + /// Sadly this has to be `pub` so we can construct these from other crates. + /// + /// We could make a constructor, but there is no point. + pub *const (), +); + +// Safety: We never actually access the pointer. +unsafe impl Send for ConstTypeId_ {} +// Safety: We never actually access the pointer. +unsafe impl Sync for ConstTypeId_ {} + +/// An object for which we can access a [`ConstTypeId_`] dynamically. +/// +/// **Do not mention this type outside of this module.** +pub trait GetConstTypeId_ { + fn const_type_id(&self) -> ConstTypeId_; +} + +/// An object for which we can get a [`ConstTypeId_`] at compile time. +/// +/// This is precisely the functionality that [`std::any::TypeId`] doesn't +/// currently have. +/// +/// **Do not mention this type outside of this module.** +pub trait HasConstTypeId_ { + const CONST_TYPE_ID_: ConstTypeId_; +} + +/// Implement [`GetConstTypeId_`] and [`HasConstTypeId_`] for one or more types. +/// +/// To avoid truly unpleasant consequences, this macro only works on simple +/// identifiers, so you can't run it on arbitrary types, or on types in other +/// modules. +#[macro_export] +macro_rules! impl_const_type_id { + { $($type:ident)* } => { + $( + impl $crate::typeid::HasConstTypeId_ for $type { + const CONST_TYPE_ID_: $crate::typeid::ConstTypeId_ = $crate::typeid::ConstTypeId_( + std::any::TypeId::of::<$type> as *const () + ); + } + + impl $crate::typeid::GetConstTypeId_ for $type { + fn const_type_id(&self) -> $crate::typeid::ConstTypeId_ { + <$type as $crate::typeid::HasConstTypeId_>::CONST_TYPE_ID_ + } + } + )* + } +} +pub use impl_const_type_id; + +#[cfg(test)] +mod test { + use assert_impl::assert_impl; + + struct Foo(usize); + struct Bar {} + + crate::impl_const_type_id! {Foo Bar} + + #[test] + fn typeid_basics() { + use super::*; + assert_impl!(Send: ConstTypeId_); + assert_impl!(Sync: ConstTypeId_); + let foo1 = Foo(3); + let foo2 = Foo(4); + let bar = Bar {}; + + assert_eq!(foo1.const_type_id(), foo2.const_type_id()); + assert_ne!(foo1.const_type_id(), bar.const_type_id()); + } +}