Make gRPC requests go through tendermint Query

[amaury1093]

closes: #XXXX

this PR:

• makes all gRPC requests go through clientCtx (ABCIQuery or Broadcast)
• on first call of each grpc method handler, the return type is saved in a map inside GrpcQueryRouter. This map is used for correctly decoding grpc responses. (see thread Make gRPC requests go through tendermint Query #8549 (comment) for more info).

Problem description

User sends a query with grpc
Tendermint Commit new blocks and tries to Commit IAVL (which causing IAVL versions map to change)
At the same time query tries to read from the same map (iavl.(*MutableTree).VersionExists) to check if requested version is exists
Node exits with fatal error: concurrent map read and map write

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[amaury1093]

Status update: as per #8549 (comment), I added a returnType map[string]reflect.Type which maps each FQ method name to its return type. This map get populated when calling dryRunMethodHandlers, but i'm not sure from where to call it.

(Note: to dry run the method handlers, we need a context.Context, and I'm passing a sdk.WrapSDKContext(ctx) right now)

I tried:

• populating returnTypes immediately on RegisterService (so that we wouldn't even need a separate dryRunMethodHandlers function) using a dummy sdk.Context. But it panicked on some nil stores.
• calling dryRunMethodHandlers in InitChain, panicking for unmarshalling empty bytes request.

I'm thinking of a way to avoid panicking on all grpc query handlers. Maybe there's a way to do it with context.CancelFunc instead of sdk.WrapSDKContext(someSdkCtx)? Some other ideas?

[aaronc]

You can pre-run these in RegisterService I think. I'll take a quick look

[aaronc]

Actually looks pretty nasty... I remember we had this issue before.

[aaronc]

Actually if this is all the same process we can just capture the return type the first time a real query runs. I know that's super hacky but it should work unless someone is running grpc stand-alone.

A more correct way would be - read grpc schema from message descriptors, then look up the return type name, then look up the actual go type from the proto type registry.

What do you think?

[amaury1093]

The grpc reflection way also seems more correct to me, however:

then look up the return type name

I can't seem to find where to find the mapping of fq method name to return type (in grpc-go code)

Actually if this is all the same process we can just capture the return type the first time a real query runs. I know that's super hacky but it should work unless someone is running grpc stand-alone.

For now I pushed this version

[aaronc]

Get the file descriptor with proto.FileDescriptor and the go type with proto.MessageType. Does that help? This involves actually parsing a FileDescriptor...

[amaury1093]

This involves actually parsing a FileDescriptor

Ah right. I would need to define some heuristics to guess the file name from the fq method name, right? Will try that, but seems hacky too...

[amaury1093]

I think it'll be hacky, we have e.g. FileDescriptor for cosmos/base/reflection/v1beta1/reflection.proto, and there's no way we can guess the file path from the fq service name (e.g. cosmos.base.reflection.v1beta1.ReflectionService), unless introducing a lot of ifs.

Maybe let's just stay with capturing the return type on 1st call?

[aaronc]

If we have access to a reflection grpc service then we can actually just use that to get the descriptors. But honestly hopefully this is just a short term fix and we deal with the concurrency issues soon. I would hate for the whole abci pipeline to be forever single threaded.

[amaury1093]

If we have access to a reflection grpc service then we can actually just use that to get the descriptors.

We'd need a client for that, it seems overkill. i also remember @clevinson running into issues with reflection because we're using gogoproto.

anyways, the current approach should be good enough for now.

[amaury1093]

Marking this R4R, the chosen method is described in the first post.

[codecov]

Codecov Report

Merging #8549 (2405701) into master (cb725e1) will decrease coverage by 0.00%.
The diff coverage is 31.03%.

@@            Coverage Diff             @@
##           master    #8549      +/-   ##
==========================================
- Coverage   61.43%   61.43%   -0.01%     
==========================================
  Files         656      656              
  Lines       37526    37533       +7     
==========================================
+ Hits        23055    23059       +4     
- Misses      12063    12065       +2     
- Partials     2408     2409       +1     

Impacted Files	Coverage Δ
baseapp/grpcserver.go	2.70% <0.00%> (+0.13%)	⬆️
client/grpc_query.go	29.16% <40.00%> (+0.90%)	⬆️
baseapp/grpcrouter.go	80.00% <50.00%> (-6.05%)	⬇️
server/grpc/server.go	62.50% <100.00%> (ø)
testutil/network/util.go	71.02% <100.00%> (ø)

[aaronc]

This all feels very unfortunate and also necessary. Let's please make it a priority to have read concurrency in the near future.

[robert-zaremba]

Left few comments. On the serialization logic we can follow up in other task. Approving to let it go.

[robert-zaremba]

Instead of using reflections, I would propose something else.

All return types are proto.Message

Instead of having a map:

methodName -> reflect.Type

we can use factory functions:

methodName -> func() proto.Message

I'm not sure where is the best place to create that functions - probably each module will need to do it itself when registering interfaces? So a bit more work, and more verbose. Not for this task, but idea for the future. If you think this make sense then we can create an issue for this.

[amaury1093]

we could do that already, but still using reflection:

- qrt.returnTypes[fqName] = reflect.TypeOf(res)
+ qrt.returnTypes[fqName] = func() proto.Message {
+   typ := reflect.TypeOf(res)
+   return reflect.New(typ.Elem()).Interface().(proto.Message)
+ }

Both are equivalent in my eyes, I don't have a preference. happy to switch to factory functions (with reflection inside) if you find them clearer.

[robert-zaremba]

I was thinking more about registering this methods in the modules when registering the query server, rather than baseapp. So that would be before even running the query server. It's more declarative - so a bit more code, but we won't use reflection.

[robert-zaremba]

shall we create a task to discuss it further?

[robert-zaremba]

I created one: #8588

[robert-zaremba]

why do we use SendHeader instead of SetHeader?

[amaury1093]

Following the docs https://github.com/grpc/grpc-go/blob/master/Documentation/grpc-metadata.md#sending-metadata-1

[robert-zaremba]

Do you know why before SetHeader was used?

[robert-zaremba]

Do you think it would be possible to store an object in the abciRes to avoid the serialization round trip? RunGRPCQuery would need to have additional parameter: serialize bool (or thous would be set in a header). If not set, we don't serialize, and instead set the value in the response directly.

[amaury1093]

abciRes comes from tendermint, so not easily.

We could create a wrapper around it, for optimization (not serializing/deserializing the same structs multiple times...) as you say. But that's a bigger refactor, and if we're talking about bigger refactors, a more sensible one would be to allow concurrent gRPC queries.

[tac0turtle]

This pr came to mind recently while talking with a few folks. Do we think it would be worth to attach our own mutex instead of routing through tendermint. The routing through tendermint seems like a quick fix but it slows down queries through tendermint and the sdk, while having our mutex would only slow down the sdk queries.

This is also following a conversation that was had a couple months ago in tendermint (tendermint/tendermint#6171). Should applications handle their own concurrency, IMO yes.

[robert-zaremba]

Since app is managing the IAVL, it makes sense to use the app for organizing IAVL access.
This way we can remove the roundtrips with "re-serialization" logic.