feat: add a shared p2p rpc client session pool to reduce rpc setup time #3152

sdbondi · 2021-08-01T13:27:37Z

Description

Adds an RPC client session pool to efficiently maintain multiple shared
RPC sessions on the p2p network.

Motivation and Context

Wallet currently establishes one or more RPC sessions per transaction. At times, during stress tests these
sessions are opened and closed many times. Each time an RPC sessions is established, protocol negotiation
and RPC handshakes must occur. These components can make use of the shared RPC client pool
to more efficiently and easily manage multiple sessions without creating situations where the target peer is
thrashed with 10s of thousands of sessions in a stress test situation.

Implementing this in the wallet is TODO.

Some RPC test utilities were moved around to make them useful to other tests, which has added some LOC

How Has This Been Tested?

Extensive unit tests

Checklist:

I'm merging against the development branch.
I have squashed my commits into a single commit.

hansieodendaal · 2021-08-02T09:46:58Z

comms/src/connection_manager/peer_connection.rs

+    /// Creates a new RpcClientPool that can be shared between tasks. The client pool will lazily establish up to
+    /// `max_sessions` sessions and provides client session that is least used.
+    #[cfg(feature = "rpc")]
+    pub fn create_rpc_client_pool<T>(&self, max_sessions: usize) -> RpcClientPool<T>


Please help me understand this:
I would have thought that calls to pub async fn connect_rpc and pub async fn connect_rpc_using_builder would call pub fn create_rpc_client_pool if a pool does not exist end then return connections from the pool only? Is that not the purpose of the pool?

I understand the thinking but unfortunately it's not that easy to do given that all clients have a different type.

Remember a pool session is per peer per protocol.

A pool of sessions has to be typed to an rpc protocol i.e. RpcClientPool<WalletBaseNodeClient>. So storing a generic list containing different types requires RpcClientPool<Box<dyn RpcPoolClient>> which actually requires setting up a BoxedRpcClientPool type. So basically, making this generic is not straight forward - I also don't think it's a particularly good solution, because:

RPC is designed to for single concerns - one concern needs one kind of rpc service. Services are small and focused. You shouldn't need to open more than one/two sessions to a particular protocol i.e. you should design your software to not need more than this (though in reality you have quite a margin for abuse as we've seen).

it is MUCH easier for the caller to get a pool and manage it, then to make a boxed dynamic list/hashmap. The domain knows about the pool it needs.

Only the wallet needs a pool currently.

How big of a pool do you need? Is one size fits all a good decision?

It has a memory and perf cost which is simply not necessary for most components to pay

Other minor concerns which add undue code complexity - When can we close the pooled sessions? Do we assume that we are never done using the pool? Do we add another call(s) to manage this pool state for everyone?

So, the wallet will get a pool, and then use it to get the connections it needs. In future, other components may need a pool so they have that available. It really depends on the lifetime of your session and requirements so it's probably a mistake to try and implement something that covers all possible domain requirements.

Understood, thanks, and agreed. The caller/client process can/should manage the sessions.

philipr-za

Looks good.

Is the plan that the domain level clients will maintain one of these for a given peer connection and recreate it if the peer connection drops? So they will handle the re-dialing if required. I can see that the maintenance of the peer connection could become a bit tricky.

sdbondi · 2021-08-02T10:32:10Z

@philipr-za Yeah, this PR takes care of the pooled management of a particular RPC session. I am implementing a domain-level component that manages the connection to a base node, which should be a matter of dialling it again if it drops and creating the shared pool. Callers can call on this to get a session from the pool without being exposed to managing the connection or needing to know about base node changes etc. Maybe that component can be made generic, but that is not the goal at the moment

philipr-za · 2021-08-02T10:32:31Z

Looks good.

Is the plan that the domain level clients will maintain one of these for a given peer connection and recreate it if the peer connection drops? So they will handle the re-dialing if required. I can see that the maintenance of the peer connection could become a bit tricky.

Your response to @hansieodendaal answered this question to some extent. The wallet will need to manage these pools as they are required by tasks so it can keep a client open for tasks that need them and once no more tasks need them close them down.

sdbondi · 2021-08-02T10:45:10Z

Just pushed up another test that I wanted in here

hansieodendaal

LGTM

Just some notes for implementing this further. We may want a domain level client that creates an RPC connection pool to have options to specify:

if RPC, in general, should take care of reconnecting dropped client connections, furthermore if the client should be exclusive or if it may be selected from a peer list;
clearing out/dropping a percentage of PRC sessions if not used for some time (e.g. pool size = 100, never drop sessions to below 10).

stringhandler

Looks good. I would ideally want the wallet to have something a bit different but I think this can be extended into this:

In order of preference I would like:
Option 1. The wallet to have only one connection pool that each protocol/service can use as a dependency, and it asks it for a connection of type T (e.g. BaseNodeRpc) to an address. i.e.
pool.get_client<TRpcClient>(address). If the rpc dance has been done already, return a client, otherwise negotiate
Option 2. The wallet have one connection pool per rpc client type. A protocol/service that needs both basenoderpc and basenodewalletrpc would have one dependency per client type it needs
Option 3. This looks like what you have, although I might be misunderstanding, which is that you have to have a pool per address and rpc type. This is useful, but I think the other two options are going to be more ergonomic

Happy to merge this, and future improvements can be built upon it

sdbondi · 2021-08-04T11:19:34Z

@mikethetike Thanks for the review - looks like we're on the same page. This pool construct forms the basis for what you have outlined.

Option 1) This has the same issues as raised here #3152 (comment) - we'll see that this is something we can do without or build non-generically (types are known) in the domain layer where a pool is needed (currently only the wallet) - PR incoming
Options 2 and 3 are coming in a subsequent domain-level PR (Update: #3159 )

aviator-app · 2021-08-04T11:56:18Z

PR queued successfully. Your position in queue is: 3

aviator-app · 2021-08-04T13:41:04Z

PR is on top of the queue now

aviator-app · 2021-08-04T14:36:11Z

PR failed to merge with reason: Some CI status(es) failed
Failed CI(s): ci/circleci: run-integration-tests

aviator-app · 2021-08-04T14:55:15Z

PR queued successfully. Your position in queue is: 8

aviator-app · 2021-08-04T21:51:48Z

PR is on top of the queue now

aviator-app · 2021-08-04T22:47:56Z

PR failed to merge with reason: Some CI status(es) failed
Failed CI(s): ci/circleci: run-integration-tests

aviator-app · 2021-08-05T04:25:51Z

PR queued successfully. Your position in queue is: 1

Adds an RPC client session pool to efficiently maintain multiple shared RPC sessions on the p2p network.

Potentially fixes GRPC connection issues we've been having

aviator-app · 2021-08-07T14:05:03Z

PR queued successfully. Your position in queue is: 2

aviator-app · 2021-08-07T14:17:42Z

PR is on top of the queue now

## Description  Adds a service responsible for wallet connectivity. This service is responsible for and abstracts any complexity in the management of the base node connections and RPC session management. This PR makes use of this service in the base node montoring service but does not "plumb" the WalletConenctivityService into the protocols. This is left as a TODO, but we should expect this to remove many lines of code and greaty simplify these protocols by removing the budren of connection management in the various wallet components. A number of simplifications on the peer connection and substream code, debatably reducing places that bugs could hide. ## Motivation and Context   Follow on from #3152, making use of the pool in the wallet base node monitoring. The rest of the protocols should follow. ## How Has This Been Tested?    Extensive service unit tests Existing tests pass Washing machine test on local wallet, with the broadcast protocol modified to use the wallet connectivity Base node RPC sessions max out at 10 each (2 wallets connected to same base node) even when pushing through 426 transactions. ![image](https://user-images.githubusercontent.com/1057902/128175630-52617736-f71f-4865-a915-6d6c9b00e8d5.png) Manually tested wallet connectivity states (Connecting -> Online | Connecting -> Offline | Connecting -> Online -> Offline -> Connecting -> Online) ## Checklist:  * [x] I'm merging against the `development` branch. * [x] I have squashed my commits into a single commit.

sdbondi force-pushed the comms-rpc-connection-pool branch 3 times, most recently from a0b082b to 2c4e82c Compare August 2, 2021 05:36

hansieodendaal reviewed Aug 2, 2021

View reviewed changes

philipr-za previously approved these changes Aug 2, 2021

View reviewed changes

sdbondi dismissed philipr-za’s stale review via 4c7544d August 2, 2021 10:44

sdbondi force-pushed the comms-rpc-connection-pool branch from 2c4e82c to 4c7544d Compare August 2, 2021 10:44

sdbondi force-pushed the comms-rpc-connection-pool branch from 4c7544d to 2cfdf7a Compare August 2, 2021 12:06

hansieodendaal previously approved these changes Aug 2, 2021

View reviewed changes

stringhandler dismissed hansieodendaal’s stale review via 2cfdf7a August 3, 2021 09:10

stringhandler previously approved these changes Aug 4, 2021

View reviewed changes

sdbondi mentioned this pull request Aug 4, 2021

feat: wallet connectivity service #3159

Merged

2 tasks

stringhandler added the mq-approved label Aug 4, 2021

stringhandler changed the title ~~feat: shared p2p rpc client session pool~~ feat: add a shared p2p rpc client session pool to reduce rpc setup time Aug 4, 2021

aviator-app bot added the mq-failed label Aug 4, 2021

aviator-app bot removed the mq-failed label Aug 4, 2021

aviator-app bot added the mq-failed label Aug 4, 2021

aviator-app bot removed the mq-failed label Aug 5, 2021

aviator-app bot added the mq-failed label Aug 5, 2021

sdbondi force-pushed the comms-rpc-connection-pool branch 3 times, most recently from 2133424 to 6573ff3 Compare August 5, 2021 11:15

sdbondi marked this pull request as draft August 6, 2021 07:26

sdbondi changed the title ~~feat: add a shared p2p rpc client session pool to reduce rpc setup time~~ DNM feat: add a shared p2p rpc client session pool to reduce rpc setup time Aug 6, 2021

sdbondi force-pushed the comms-rpc-connection-pool branch 7 times, most recently from 5b5a0c7 to bae7360 Compare August 6, 2021 11:04

sdbondi changed the title ~~DNM feat: add a shared p2p rpc client session pool to reduce rpc setup time~~ feat: add a shared p2p rpc client session pool to reduce rpc setup time Aug 6, 2021

sdbondi marked this pull request as ready for review August 6, 2021 12:59

sdbondi force-pushed the comms-rpc-connection-pool branch 2 times, most recently from 5ae87d9 to 0f1dc29 Compare August 6, 2021 13:00

sdbondi added 6 commits August 7, 2021 17:57

feat: shared p2p rpc client session pool

05fb77f

Adds an RPC client session pool to efficiently maintain multiple shared RPC sessions on the p2p network.

fix: better method for getting an open port in cucumber tests

2d9f3a6

fix(cucumber): update to @grpc/grpc-js library

04a0b3f

Potentially fixes GRPC connection issues we've been having

misc: update package lock

a6950bf

fix(cucumber): retry grpc connect before giving up

0a70ee1

fix(cucumber): retry wallet grpc connections

765d884

sdbondi force-pushed the comms-rpc-connection-pool branch from 0f1dc29 to 765d884 Compare August 7, 2021 14:01

stringhandler approved these changes Aug 7, 2021

View reviewed changes

aviator-app bot removed the mq-failed label Aug 7, 2021

aviator-app bot merged commit 778f951 into tari-project:development Aug 7, 2021

sdbondi deleted the comms-rpc-connection-pool branch August 8, 2021 06:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add a shared p2p rpc client session pool to reduce rpc setup time #3152

feat: add a shared p2p rpc client session pool to reduce rpc setup time #3152

sdbondi commented Aug 1, 2021 •

edited

Loading

hansieodendaal Aug 2, 2021 •

edited

Loading

sdbondi Aug 2, 2021 •

edited

Loading

hansieodendaal Aug 2, 2021

philipr-za left a comment

sdbondi commented Aug 2, 2021

philipr-za commented Aug 2, 2021

sdbondi commented Aug 2, 2021

hansieodendaal left a comment

stringhandler left a comment

sdbondi commented Aug 4, 2021 •

edited

Loading

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 5, 2021

aviator-app bot commented Aug 7, 2021

aviator-app bot commented Aug 7, 2021

feat: add a shared p2p rpc client session pool to reduce rpc setup time #3152

feat: add a shared p2p rpc client session pool to reduce rpc setup time #3152

Conversation

sdbondi commented Aug 1, 2021 • edited Loading

Description

Motivation and Context

How Has This Been Tested?

Checklist:

hansieodendaal Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

sdbondi Aug 2, 2021 • edited Loading

Choose a reason for hiding this comment

hansieodendaal Aug 2, 2021

Choose a reason for hiding this comment

philipr-za left a comment

Choose a reason for hiding this comment

sdbondi commented Aug 2, 2021

philipr-za commented Aug 2, 2021

sdbondi commented Aug 2, 2021

hansieodendaal left a comment

Choose a reason for hiding this comment

stringhandler left a comment

Choose a reason for hiding this comment

sdbondi commented Aug 4, 2021 • edited Loading

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 4, 2021

aviator-app bot commented Aug 5, 2021

aviator-app bot commented Aug 7, 2021

aviator-app bot commented Aug 7, 2021

sdbondi commented Aug 1, 2021 •

edited

Loading

hansieodendaal Aug 2, 2021 •

edited

Loading

sdbondi Aug 2, 2021 •

edited

Loading

sdbondi commented Aug 4, 2021 •

edited

Loading