Add support for VM gRPC services #3294
Conversation
joshua-kim
force-pushed
the
cmux
branch
5 times, most recently
from
56ba07f
to
a67b617
Compare
| message HttpResponse { | ||
| // header is sent to synchronize any header modifications from the plugin | ||
| // process | ||
| repeated Element header = 1; |
There was a problem hiding this comment.
grpc requires a header to indicate the end of an rpc, which is directly set onto the map in the plugin process (ref). We need to propagate this update back into avalanchego (I believe current rpcchainvm is not handling plugin-side header updates correctly).
joshua-kim
force-pushed
the
cmux
branch
2 times, most recently
from
e59b451
to
e0a43d2
Compare
| return "", nil, err | ||
| } | ||
|
|
||
| if resp.ServiceName == "" && resp.ServerAddr == "" { |
There was a problem hiding this comment.
(No action required) Is it possible that one of these is empty and the other not?
There was a problem hiding this comment.
We're doing this instead of comparing against the zero-value of the response because proto responses auto-embed protoimpl.MessageState which cannot be compared.
If one of these was empty and the other wasn't, it would be because the vm explicitly tried to register a grpc service with an empty service name which I think should be invalid (since grpc calls take the form of service + method).
There was a problem hiding this comment.
If either one being empty is invalid, does that suggest || instead of &&?
| @@ -363,6 +363,30 @@ func (vm *VMServer) CreateHandlers(ctx context.Context, _ *emptypb.Empty) (*vmpb | |||
| return resp, nil | |||
| } | |||
|
|
|||
| func (vm *VMServer) CreateGRPCService(ctx context.Context, _ *emptypb.Empty) (*vmpb.CreateGRPCHandlerResponse, error) { | |||
| serviceName, handler, err := vm.vm.CreateGRPCService(ctx) | |||
There was a problem hiding this comment.
(No action required) Why is a check required for serviceName and serviceHandler being empty in vm_client.go:CreateGRPCService but not here?
There was a problem hiding this comment.
That looks like a bug. I think it's not caught in CI because the only vm that uses rpcchainvm in our testing as part of this PR does register a grpc service.
|
|
||
| wg.Wait() | ||
|
|
||
| require.NoError(stream.CloseSend()) |
There was a problem hiding this comment.
(No action required) Will the stream be closed automatically on going out of scope (i.e. this explicit close is superfluous)? If not, maybe ginkgo.DeferCleanup is suggested?
Added |
|
The upgrade failure is legit - I'm guessing you might need to rebase? From the logs: |
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
I think my coreth branch fell behind master. Gonna rebase my coreth branch again. |
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
Signed-off-by: Joshua Kim <[email protected]>
|
This PR has become stale because it has been open for 30 days with no activity. Adding the |
Why this should be merged
Adds native support for GRPC services being registered over the avalanchego http server. This adds support for streaming via grpc as an alternative to websockets.
How this works
h2c's http handler to support plaintext http2Handlerpc that includes headersrequest.Bodyso that the plugin process can stream the body instead of the current behavior which usesio.ReadAllwhich may not be possible in the case of a streaming rpc.How this was tested
Added a e2e test for unary + streaming rpcs over rpcchainvm