server: Extend methods with optional connection context

[lexnv]

No description provided.

[niklasad1]

/cc @jsdw so basically this is the grumble

we expose now two context's here one conn_ctx which isn't exclusively connection stuff and then ctx which is completely controlled by the user.

however, the ctx is shared state per "rpc per server" and conn_ctx is per connection....

[jsdw]

Yeah ok I see!

My general grumble is that I think we should have some way to pass generic data from middleware or whatever into RPC calls. If we had that then we wouldn't need to expose eg the internal connection ID as a hardcoded thing, because the user could just create their own connection ID counter in middleware and pass that in.

Methods might want access to a bunch of other stuff from middleware that we haven't yet exposed or forseen, and I wouldn't want us to always have to work around it by trying to provide those things in a hardcoded way; I'd rather that we foudn a way to solve the problem generically :)

A couple of ideas for how:

1. Have eg a type map (ie map from type to value) that one can attach arbitrary data into in middlewares and then pull out in the method if they have access to the relevant type, and perhaps this sort of thing could be exposed via a per connection context.
2. Make jsonrpsee server generic over some Context type that is then passed through middlewares and exposed in handlers and can be whatever concrete type the user needs. I could see that being way harder to add though)

But @niklasad1 you've mentioned that maybe this is possible via low level APIs so perhaps there's a way to achieve this already?

[lexnv]

For the first idea, I've been exploring with a custom type as field to the PerConnectionContext: Arc<Mutex<anymap::Map<dyn anymap::any::Any + Send + Sync>>>. This should allow users to insert in the map any type they want.

I might be missing something here, but the trick part with this comes from the fact that the PerConnectionContext is constructed as the last step in our RpcServiceT middleware. This means that users will only access it from methods, but cannot alter its state from another RpcServiceT middleware.
From my perspective, if the PerConnectionContext can only be accessed / altered from within methods, I think this brings the same usefulness as connection: usize. Since chainHead_* methods could store internally a Map<connection, Data>.

jsonrpsee/server/src/middleware/rpc/mod.rs

Lines 42 to 50 in 736dd65

	pub trait RpcServiceT<'a> {
	/// The future response value.
	type Future: Future<Output = MethodResponse> + Send;
	/// Process a single JSON-RPC call it may be a subscription or regular call.
	/// In this interface they are treated in the same way but it's possible to
	/// distinguish those based on the `MethodResponse`.
	fn call(&self, request: Request<'a>) -> Self::Future;
	}

For this to be useful outside the method call; we'd probably need to extend the call function to include the (per connection) context : fn call(&self, request: Request<'a>, context: PerConnectionContext) -> Self::Future;. I do remember this was the original design for the middleware, but I think we had some difficulties implementing it?
And I think an extension of this is the second idea, however I don't know how much harder that is to implement.

[niklasad1]

this is tricky and I don't think it's a nice way to solve currently generic right now.

so the flow is like this:

let shared_state = Arc::new(Mutex::(HashMap::new::<ConnectionId, State>()));
let rpc_api = ChainHeadRpc::new(shared_state.clone());
// one could also write middleware but no way to share that middleware with the actual RPC implementation
// other then inject connection ID as params in the JSON-RPC call itself
// such as having an extra param: Option<ConnectionId>
start_rpc(rpc_api, shared_state);

#[rpc]
trait ChainHeadRpcApi {
   #[method(name = "chainHead_unstable_call")] 
   fn f(&self, params: Vec<u8>) -> String;
}

impl ChainHeadRpcApi for ChainHeadRpc {
   fn f(&self, params: Vec<u8>) -> String {
        // no way to know which connection made the call
        // but we can access the HashMap as shared state here
        todo!(); 
    }
}

fn start_rpc(rpc_api, shared_state) {
     for conn in server.accept() {
         // could also enable middleware but the state can't 
         // really be shared per connection in the rpc_api...
         shared_state.lock.insert(conn.id, State);
         tokio::spawn(upgrade_to_tower_service(conn));
      }
}

[niklasad1]

Thus, it would be neat to have a have closure similar for the context/user defined data in the rpc API such that users can decide whether the state for rpc impl should be shared or per connection...

[niklasad1]

I would rather re-name this to PerConnectionContext or something to easily distinguish from the other context

[niklasad1]

In the proc macros I'm not convinced that should expose ConnectionContext.

Just connection_id: usize should be sufficient and because everything internally has connection_id can't we just require everything to expose the connection ID?

Like the rpc endpoint impl shouldn't really need to know the max_response_size anyway.

/// variant one
#[rpc(server)]
trait Rpc {
    async bar(param1: usize, param2: usize, connection_id: usize) -> String;
}

/// variant two
#[rpc(server, with_context)]
trait Rpc {
    async bar(param1: usize, param2: usize, per_conn: PerConnectionContext) -> String;
}

[lexnv]

Yep, having #[rpc(server, with_context)] with-context attribute at the RPC macro level makes sense for me. We lose the ability to register individual methods other than raw_methods, but I don't think that would be a common use-case. And it could still be avoided by implementing 2 rpc modules, one without with-context and the other with :D

[niklasad1]

so, with_context shouldn't be needed if we decide to expose connection_id globally...

[niklasad1]

another option is to add new MethodKind here such as:

RawMethod = Arc<dyn Send + Sync + Fn(Id, Params, ConnectionContext) -> MethodResponse>

then let folks spawn futures and stuff them selves if they need and just provide a new attribute in the procs macros as you already did.

then register_blocking, async, etc are not needed.

[niklasad1]

then we don't have break the existing APIs

[lexnv]

That makes sense, I've implemented that in: https://github.com/paritytech/jsonrpsee/pull/1297/files#diff-dc533588250ea236f580d7fdaf87cdfce2ec80da031de9067b1088465a555998R64

One downside with doing a RawMethod like this is that we lose the ability to have:

• async methods (since this is a SyncMethod that receives an extra connection ID)
• blocking methods (since the server-context is passed differently to a blocking method -- its need to be Arc<Context> instead of &Context so our callbacks are different)

This makes me wonder if I should also add the counter-part RawAsyncMethod and another register_raw_blocking. I think a single RawMethod would solve the connection ID exposure to complete the chainHead issue in substrate; but it might require us to extend it in the future.

At some point in the future , if we choose to implement RawAsyncMethod, it might be more beneficial to make a breaking change and add the connection Id to every method, for now I think we could avoid that

[niklasad1]

Closing this replaced by #1297