Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Access to raw unmarshalled bytes from grpc methodhandler #6251

Closed
thefallentree opened this issue May 3, 2023 · 10 comments
Closed

Access to raw unmarshalled bytes from grpc methodhandler #6251

thefallentree opened this issue May 3, 2023 · 10 comments
Assignees
Labels
Type: Feature New features or improvements in behavior

Comments

@thefallentree
Copy link

thefallentree commented May 3, 2023

Use case(s) - what problem will this feature solve?

currently in the grpc generated code, only the unmarshalled struct is accessible by the method handler , this is because
the server side code doesn't pass in the decompressed rawbytes as seen here

https://github.com/grpc/grpc-go/blob/47b3c5545c4d9bef0d42eb1ced7afb313dd7aa92/server.go#LL1336C12-L1336C12

the suggestion is to inject this into the context to make it available if the method handler wants it

ctx := NewContextWithServerTransportStream(stream.Context(), stream)
# inject d to the context
 ctx = context.WithValue(dataKey, d)
reply, appErr := md.Handler(info.serviceImpl, ctx, df, s.opts.unaryInt)

there is no way to get the raw unmarshalled bytes

Proposed Solution

add it to the ctx invoked to the MethodHandler similar to the new access to ServerStream is implemented

Alternatives Considered

Additional Context

@thefallentree thefallentree added the Type: Feature New features or improvements in behavior label May 3, 2023
@easwars
Copy link
Contributor

easwars commented May 4, 2023

Could you please explain the use case for this? Thanks.

@thefallentree
Copy link
Author

@easwars the use case is like this
-> we have a raft system with grpc endpoints (distributed consensus) and we want to replicate all incoming requests through raft consensus. The Raft layer is like a binary log, it just needs raw bytes to replicate around.

so on the incoming request path we have today

  1. grpc layer received bytes from ServerStream, and call codec to deserialize it into protobuf
  2. our request handler serialize the entire request into bytes then replicate the byte,
  3. later on our raft replication handler it then deserialize the bytes back again to process it

As you can see we can get rid of the serialization cost if we have access to raw bytes from within the grpc request handler

@thefallentree thefallentree removed their assignment May 4, 2023
@easwars
Copy link
Contributor

easwars commented May 4, 2023

Would a stats handler work for you to get to the serialized bytes?

See: https://pkg.go.dev/google.golang.org/grpc@v1.54.0/stats#Handler and https://pkg.go.dev/google.golang.org/grpc@v1.54.0/stats#InPayload

@thefallentree
Copy link
Author

Would a stats handler work for you to get to the serialized bytes?

See: https://pkg.go.dev/google.golang.org/grpc@v1.54.0/stats#Handler and https://pkg.go.dev/google.golang.org/grpc@v1.54.0/stats#InPayload

We need access to raw bytes from within the request handler: having it in a stats handler then we would need to have some sort of way to look up raw bytes based on some filed in the requests, which wouldn't work as generic

@easwars
Copy link
Contributor

easwars commented May 5, 2023

Would using a custom codec work for you? See: https://pkg.go.dev/google.golang.org/grpc@v1.55.0/encoding#Codec. The implementation of the proto codec is here: https://github.com/grpc/grpc-go/blob/master/encoding/proto/proto.go. You could create a custom codec which does proto marshaling/unmarshaling and also replicate the raw bytes.

Attaching the raw bytes in the context (to make it available to the request handler) will make it hard/impossible to re-use that memory.

@thefallentree
Copy link
Author

Would using a custom codec work for you? See: https://pkg.go.dev/google.golang.org/grpc@v1.55.0/encoding#Codec. The implementation of the proto codec is here: https://github.com/grpc/grpc-go/blob/master/encoding/proto/proto.go. You could create a custom codec which does proto marshaling/unmarshaling and also replicate the raw bytes.

Attaching the raw bytes in the context (to make it available to the request handler) will make it hard/impossible to re-use that memory.

Yeah: but doing that requires registering a new codec, change all client call sites to use the new codec , also we will need to change the Request protobuf to add a byte field to replicate into. It seems just very big effort compare to do in the server code.

First of all, I think a server option should be guarding this logic, not all servers needs access to the raw bytes, hence it's best default off.
second, turn on this flag should just copys the raw byte into the context, so there is no reuse issues.

@easwars
Copy link
Contributor

easwars commented May 5, 2023

@dfawley : Thoughts?

@dfawley
Copy link
Member

dfawley commented May 16, 2023

We need access to raw bytes from within the request handler: having it in a stats handler then we would need to have
some sort of way to look up raw bytes based on some filed in the requests, which wouldn't work as generic

I think this could work:

  1. your stats handler's TagRPC method would add a value to the context.
  2. your stats handler's HandleRPC method would add the raw request bytes to that value for stats.InPayload types.
  3. your application would access that same context value in the method handler.

Note that no context-based solution here would work for streaming RPCs. Really, this is a parameter of the message, and not the RPC. But our messages are just interface{} and cannot be changed, as they need to be coerced to specific proto struct types to be used with the generated API. If you are using the native API instead of the proto generated code, you could return a type that is able to produce both raw bytes and decoded message structs, but that makes the more normal uses more difficult.

Anyway I don't think there's much we can do here in gRPC itself, so I'm going to close this for now.

@dfawley dfawley closed this as completed May 16, 2023
@thefallentree
Copy link
Author

thefallentree commented May 17, 2023 via email

@dfawley
Copy link
Member

dfawley commented May 22, 2023

is there at least a plan to make it easier to provide server implementation?

I'm not sure what you're getting at with this. If you want to rewrite gRPC, then you will have to fork it. If you want some specific customization, then we have several mechanisms you can use (https://pkg.go.dev/google.golang.org/grpc#InTapHandle, https://pkg.go.dev/google.golang.org/grpc#StatsHandler, https://pkg.go.dev/google.golang.org/grpc#StreamInterceptor / https://pkg.go.dev/google.golang.org/grpc#UnaryInterceptor, and https://pkg.go.dev/google.golang.org/grpc#UnknownServiceHandler). If you have a proposal for some other way of customizing, feel free to suggest it, but with all these various hooks in place, we don't get many requests for additional functionality.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

No branches or pull requests

3 participants