Explore gRPC queries concurrency

[amaury1093]

Summary

We should allow concurrent gRPC queries (state reads).

Problem Definition

After #8549, all gRPC queries are routed to the ABCI system. The current ABCI implementation uses global locks across all requests, so only one query can be processed at a time (also see abci notes).

Proposal

Explore how to have concurrent gRPC read queries.

Notes

• With the current implementation, we also use an ugly hack to store each gRPC method handler return type: Make gRPC requests go through tendermint Query #8549 (comment). If we keep this hack, let's also think about how to make it cleaner: Don't use reflection when creating gRPC responses #8588

cc @robert-zaremba @aaronc

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[adasari]

@robert-zaremba - not sure if it is right place to ask question. i looked into #8549 merge request, why is grpac-gateway is not used in first place to support both grpc and http end points ? which eliminates maintaining the mappings and return types by yourself.

Could you share more details about the ticket ? i will contribute if i understand the problem. thanks.

[danil-lashin]

As I see the only possible trouble happens when we trying to construct new ImmutableTree instance simultaneously with committing old one. So RWMutex in Store will be a simple and valid solution. I would be happy to make a PR if nobody mind.

[robert-zaremba]

@adasari - with current design all queries must go through ABCI, otherwise we have some subtle inconsistencies. ABCI is sequential, so it's a kind of a bottleneck.

[albertchon]

Not sure if this is still relevant, but we've faced this issue in the past (fatal error: concurrent map read and map write) and @xlab made a quick fix by using wrapping the MutableTree in a RW mutex InjectiveLabs/iavl@df2d577

[ethanfrey]

The current ABCI implementation uses global locks across all requests, so only one query can be processed at a time (also see abci notes).

This is by design. There are 3 connections between tendermint and the abci app (deliver, check, query) and each one is processed sequentially. However, the 3 can run in parallel.

In the app, each has access to a different state/store so this should avoid race conditions. We clearly cannot remove the lock for deliver and check, but it may be possible to do so with query. However, all app reasoning has been made in a sequential model, so this may introduce some race conditions down the line.

I know of no clear problem I can point at, but I would request involving the people that made the original design in such a discussion. Especially @ebuchman but maybe he can suggest more. Adding concurrency into a system designed with sequential execution in mind should not be taken lightly. Let's make sure it cannot introduce any subtle errors first

[fedekunze]

cc @sunnya97 @ValarDragon does ABCI++ architecture solve this?

[aaronc]

cc @sunnya97 @ValarDragon does ABCI++ architecture solve this?

I'm pretty sure the issue is more about read write concurrency at the storage layer within the SDK. It should be fixed by upgrading our state storage layer which is in progress separately (#8430).

[ValarDragon]

I don't think ABCI++ touches on this, its only dealing with how a single ABCI connection is used within consensus. This is more about managing distinct ABCI connections

[tac0turtle]

closing this in favour of #10859. There is still something blocking parallel queries tho, outlined in the mentioned issue.