Re: [PATCH proxmox 01/20] router: introduce shared state

[Robert Obkircher <r.obkircher@proxmox.com>]

> API handlers often need access to long-lived application data such as
> configuration, caches or client handles. So far the only way to get
> there is a global static, which hides the actual dependencies of a
> handler and makes testing awkward.
> 
> Add a type-keyed registry that a server fills once during startup,
> together with an accessor on RpcEnvironment so that handlers can reach
> it. The State<T> newtype wraps values that come from the registry,
> which allows telling them apart from regular API parameters.
> 
> Signed-off-by: Lukas Wagner <l.wagner@proxmox.com>
>
> diff --git a/proxmox-router/src/cli/environment.rs b/proxmox-router/src/cli/environment.rs
> index c85105a7..c9aefb75 100644
> --- a/proxmox-router/src/cli/environment.rs
> +++ b/proxmox-router/src/cli/environment.rs
> @@ -5,7 +5,7 @@ use serde_json::Value;
>  
>  use proxmox_schema::ApiType;
>  
> -use crate::{RpcEnvironment, RpcEnvironmentType};
> +use crate::{RpcEnvironment, RpcEnvironmentType, SharedStateRegistry};
>  
>  /// [`RpcEnvironment`] implementation for command line tools.
>  ///
> @@ -15,6 +15,7 @@ use crate::{RpcEnvironment, RpcEnvironmentType};
>  pub struct CliEnvironment {
>      result_attributes: Value,
>      auth_id: Option<String>,
> +    shared_state_registry: Option<SharedStateRegistry>,
>      pub(crate) global_options: HashMap<TypeId, Box<dyn Any + Send + Sync + 'static>>,
>  }
>  
> @@ -23,6 +24,11 @@ impl CliEnvironment {
>          Default::default()
>      }
>  
> +    /// Set the shared state registry for this environment.
> +    pub fn set_shared_state_registry(&mut self, registry: SharedStateRegistry) {
> +        self.shared_state_registry = Some(registry);
> +    }
> +
>      /// Borrow a global option by type.
>      ///
>      /// Returns `None` if the option type was not registered or no value was provided on the
> @@ -98,4 +104,8 @@ impl RpcEnvironment for CliEnvironment {
>      fn get_auth_id(&self) -> Option<String> {
>          self.auth_id.clone()
>      }
> +
> +    fn shared_state(&self) -> Option<&SharedStateRegistry> {
> +        self.shared_state_registry.as_ref()
> +    }
>  }
> diff --git a/proxmox-router/src/lib.rs b/proxmox-router/src/lib.rs
> index da2f018f..df225d25 100644
> --- a/proxmox-router/src/lib.rs
> +++ b/proxmox-router/src/lib.rs
> @@ -16,6 +16,7 @@ mod permission;
>  mod router;
>  mod rpc_environment;
>  mod serializable_return;
> +mod shared_state;
>  
>  #[doc(inline)]
>  #[cfg(feature = "server")]
> @@ -25,6 +26,7 @@ pub use permission::*;
>  pub use router::*;
>  pub use rpc_environment::{RpcEnvironment, RpcEnvironmentType};
>  pub use serializable_return::SerializableReturn;
> +pub use shared_state::{SharedStateRegistry, State};
>  
>  // make list_subdirs_api_method! work without an explicit proxmox-schema dependency:
>  #[doc(hidden)]
> diff --git a/proxmox-router/src/rpc_environment.rs b/proxmox-router/src/rpc_environment.rs
> index 8ce2d99d..e065504a 100644
> --- a/proxmox-router/src/rpc_environment.rs
> +++ b/proxmox-router/src/rpc_environment.rs
> @@ -2,6 +2,8 @@ use std::any::Any;
>  
>  use serde_json::Value;
>  
> +use crate::SharedStateRegistry;
> +
>  /// Helper to get around `RpcEnvironment: Sized`
>  pub trait AsAny {
>      fn as_any(&self) -> &(dyn Any + Send);
> @@ -45,6 +47,11 @@ pub trait RpcEnvironment: Any + AsAny + Send {
>      fn get_client_ip(&self) -> Option<std::net::SocketAddr> {
>          None // dummy no-op implementation, as most environments don't need this
>      }
> +
> +    /// Return a reference to the shared state registry.
> +    fn shared_state(&self) -> Option<&SharedStateRegistry> {
> +        None
> +    }
>  }
>  
>  /// Environment Type
> diff --git a/proxmox-router/src/shared_state.rs b/proxmox-router/src/shared_state.rs
> new file mode 100644
> index 00000000..14a539af
> --- /dev/null
> +++ b/proxmox-router/src/shared_state.rs
> @@ -0,0 +1,46 @@
> +//! Type-keyed state that API handlers can request as a parameter.
> +
> +use std::any::{Any, TypeId};
> +use std::collections::HashMap;
> +
> +use anyhow::{Error, bail};
> +
> +/// Registry of state values, keyed by their type.
> +///
> +/// It holds at most one value per type. Values are registered with
> +/// [`register`](SharedStateRegistry::register) before the registry is handed over to the API
> +/// environment, from where handlers can access them through
> +/// [`RpcEnvironment::shared_state`](crate::RpcEnvironment::shared_state). This allows passing
> +/// application context to handlers without resorting to globals.
> +#[derive(Default)]
> +pub struct SharedStateRegistry {
> +    map: HashMap<TypeId, Box<dyn Any + Send + Sync>>,
> +}

We could get rid of some indirection by replacing the map with a
Box<&dyn SharedState>. Probably not worth it, though.

trait SharedState { fn lookup(&self, id: TypeId) -> &dyn Any; }

struct PdmState { a: A, b: B }

impl SharedState for PdmState {
    fn lookup(&self, id: TypeId) -> &dyn Any {
        if id == TypeId::of::<A>() { &self.a }
        else if id == TypeId::of::<B>() { &self.b }
        else { unreachable!() }
    }
}


> +
> +impl SharedStateRegistry {
> +    /// Get a clone of the registered value of type `T`, if there is one.
> +    pub fn lookup<T: 'static + Send + Sync + Clone>(&self) -> Option<T> {
> +        self.map
> +            .get(&TypeId::of::<T>())
> +            .and_then(|s| s.downcast_ref())
> +            .cloned()
This could .expect("value must have correct type for key").

-- 
Robert Obkircher <r.obkircher@proxmox.com>