PoC: gRPC server standardization#1742
PoC: gRPC server standardization#1742Mirko-von-Leipzig wants to merge 16 commits intomirko/build-codegenfrom
Conversation
179fae4 to
7b31bf4
Compare
| return Ok(()); | ||
| } | ||
|
|
||
| let status = Command::new("rustfmt") |
There was a problem hiding this comment.
we should try to avoid calling external funbi aries, particularly when it's impossible to pass on flags (i.e. which version / edition does it expect?)
I suggest to use something like prettyplease for generated code https://github.com/drahnr/expander/blob/master/src/lib.rs#L239-L246
There was a problem hiding this comment.
I used prettyplease originally, and it gave notably worse output - and also worse dx because if you enter the generated file manually and hit save, your ide may autofmt it again.
I figured using rustfmt would be much better? I don't really care what version or edition it uses - its just to make it legible. It is actually possible to pass edition and other flags in; I removed it because I thought it added no value.
There was a problem hiding this comment.
It will error if you're using a version that doesn't know about certain idioms, i.e. 2018 will bail on let Some(foo) = bar else {..};
| )) | ||
| })?; | ||
| Ok(signature) | ||
| } |
There was a problem hiding this comment.
Minor stylistic proposal: I find it a bit awkward to have many traits, I'd rather have a single, parameterized trait that uses a concrete type as parameter:
impl WireCodec for MySuperDuperInteraction {
type Input = ..;
type Output = ..;
fn decode_input(grpc::input::Type) -> Result<Self::Input> {
..
}
fn encode_output(Self::Output) -> Result<grpc::output::Type> {
..
}
}
impl GrpcInteraction<MySuperDuperInteraction> for ValidatorServer {
async fn handle(&self, input: MySuperDuperInteraction::Input) -> tonic::Result<Self::Output> {
..
}
There was a problem hiding this comment.
Agree, we can achieve the same separation of concerns with fewer generated types
There was a problem hiding this comment.
I'm not quite understanding why you think it will result in less types? You can either use more traits, and slightly less traits and many structs and impls. The trait variant is much less code, and less indirection imo.
But maybe I've missed an approach? @SantiagoPittella I don't quite follow your proc macro suggestion -- on what do you place the proc macro?
@drahnr I actually had your suggestion implemented here, in commit d9fd24.
There was a problem hiding this comment.
But I agree simplifying would be great. Create a new thread in one of the files with a sketch of the code you want to generate, and we can hash it out
| fn generate_mod_rs(dst_dir: impl AsRef<Path>) -> std::io::Result<()> { | ||
| let mod_filepath = dst_dir.as_ref().join("mod.rs"); | ||
| // I couldn't find any `codegen::` function for `mod <module>;`, so we generate it manually. | ||
| let mut modules = Vec::new(); |
There was a problem hiding this comment.
There should be Module::new("we_are_here").get_or_new_module("foo")
|
I like how methods can be added and implemented with this approach, but the code generation part is difficult to follow. I have some doubts about maintainability of this, though on the other hand I assume that this is not supposed to be changed frequently. This changes + an extensive explanation of the generation should do the trick IMO. Cab probably achieve similar results without the codegen part, defining a single trait like /// Marker trait for gRPC method descriptors.
trait MethodDescriptor {
type Request;
type Response;
}
/// The standard decode -> handle -> encode pipeline.
#[tonic::async_trait]
trait GrpcMethod<M: MethodDescriptor>: Send + Sync {
type Input;
type Output;
fn decode(request: M::Request) -> tonic::Result<Self::Input>;
fn encode(output: Self::Output) -> tonic::Result<M::Response>;
async fn handle(&self, input: Self::Input) -> tonic::Result<Self::Output>;
async fn execute(&self, request: M::Request) -> tonic::Result<M::Response> {
let input = Self::decode(request)?;
let output = self.handle(input).await?;
Self::encode(output)
}
}Also, I'm not sure how streams will be accommodated into this pipeline. |
|
@Mirko-von-Leipzig I suppose this relates to this issue #1528 (comment) Have you thought about how it might impact the ConversionError? Should we hold off on the ConversionError conversation while this poc is going on? |
|
@sergerad yeah this overlaps -- but I think we can begin working on that in parallel if we want. I've given some more thought to the API I'd like -- but I don't actually know how to achieve it. I'll write that down in the issue. |
We have a couple of options; it's not much more complex than the normal method - we just need to decide what to do with the stream type. The trait tonic::BlockProducerApi {
type MempoolSubscriptionStream: tonic::codegen::tokio_stream::Stream<
Item = tonic::Result<MempoolEvent>
> + Send + Sync + 'static;
async fn mempool_subscription(request: ...) -> tonic::Result<Self::MempoolSubscriptonStream>);
}Some options:
|
|
I am in favor of splitting the concerns of type mapping to be split from the RPC endpoint, which would allow re-use of the encoding across types. I'd propose two new traits. This is so boring it's hardly worth typing it out. The question to me is how we want to pass the in pub trait WireCodecIn {
type GrpcInput;
fn decode_input(input: Self::GrpcInput) -> tonic::Result<Self>;
}
pub trait WireCodecOut {
type GrpcOutput;
fn encode_output(output: Self) -> tonic::Result<Self::GrpcOutput>;
}
pub trait GrpcInteraction<Input: WireCodecIn, Output: WireCodecOut> {
async fn handle(&self, input: Input) -> tonic::Result<Output>;
async fn full(&self, request: <M as WireCodecIn>::GrpcInput) -> tonic::Result<<M as WireCodecOut>::GrpcOutput> {
let input = <Input as WireCodecIn>::decode_input(request)?;
let output = self.handle(input).await?;
<Output as WireCodecOut>::encode_output(output)
}
}
pub struct RpcEndpointChicken;
impl GrpcInteraction<FetchEggs, SomeOrNone> for RpcEndpointChicken {
async fn handle(&self, input: Input) -> tonic::Result<Output> {
..
}
}What I do like about it is the modularity and the potential to opt-in to custom impls of pieces of it. |
|
Re streaming: I don't have a preference. Being able to inject input streams/mocking should be a concern, but I am not convince the protobuf layer is the right layer of mocking. |
I had pretty much exactly that here, in commit d9fd24. The intention is to have the codec stuff reuseable, but having tried it out I think having the |
|
I am a sucker for marker structs, so I'd favor the above, focusing on the pattern across RPC calls, plugging types. |
Agree. Also, this is a great fit for the We could do something like: |
|
Re: marker structs - I also like them; which is why the first attempt used them. However after toying with it for a while I had some realisations. imo the strength of marker structs is that you can define one main trait and then get a bunch of stuff for free with the generic markers. So you can define a struct and get the trait impl for free with much less boilerplate. This advantage doesn't really apply here though, because we're code generating it all. The marker structs have some downsides:
As a summary: we use generics + markers to reduce code duplication and boilerplate as a form of codegen. However, we've already got actual codegen to do this. I was sort of on the fence before, but I'm almost solidly against this now 😅 Regarding the codec stuff.. I think we definitely want a |
Proof of concept for gRPC server implementations.
Each gRPC method gets its own trait (generated by
build.rs):The implementation is not fully complete; I don't support the mempool subscription stream here for example.
The comments in thebuild.rsare also outdated and reference an initial implementation I had.Some benefits of doing this, over implementing the tonic generated traits:
GrpcDecodeandGrpcEncodetraits as a follow-on, which I think will let us be more consistent with our errors here.Server::public_facingandServer::internal, which changes how errors are encoded (i.e. hide internal errors for public facing).I've implemented the validator api as an example.
Looking for feedback if this is worth pursuing and cleaning up :) (imo yes, but maybe its not worth it).