Reuse Greenlight connection #273
Conversation
| gl_client: Mutex::new(None), | ||
| node_client: Mutex::new(None), |
There was a problem hiding this comment.
I wonder if we can get away with lazy initializing the connection. That would remove the need for the Mutex, since the grpc stubs are cloneable and will manage interior mutability (muxing requests onto the single connection).
I'm not yet familiar enough with the tonic API, but I'm sure we can use the Endpoint struct with its connect_lazy method.
There was a problem hiding this comment.
If its lazy-initialized, doesn't that mean a user action could incur an extra connection-opening time penalty?
There was a problem hiding this comment.
No, the connection initialization will have to be anyway. Also re-connection happens from time to time automatically.
There was a problem hiding this comment.
The schedule method connects to the scheduler and to the node gRPC endpoint.
I think if we remove the scheduler call (as we plan to) and use connect_lazy instead of connect here:
https://github.com/Blockstream/greenlight/blob/main/libs/gl-client/src/node/mod.rs#L103
Then we might be able to do it. But we also need that the new connect method will return the Client and not Result that might contain error.
| let mut node_client = self.node_client.lock().await; | ||
| if node_client.is_none() { |
There was a problem hiding this comment.
Looks like the recently added OnceLock in Rust 1.70 will be a good choice to init on demand here.
There was a problem hiding this comment.
OneLock doesn't support async initialization as we need here. Also we are calling the scheduler which might return error where in that case we want to be able to try and initialize the client again. OneLock give only one shot to set the value.
There was a problem hiding this comment.
What about some general locking pattern like
if it's not None, return it
lock a mutex
if it's not None, return it
initialize it and return it
There was a problem hiding this comment.
Checking if not None still accesses the shared memory which requires lock. We can though use RWLock as most of the access attempts are for read.
roeierez
force-pushed
the
gl-connection-reuse
branch
from
7ec6b96
to
47c9752
Compare
| let scheduler = | ||
| Scheduler::new(self.signer.node_id(), self.sdk_config.network.into()).await?; |
There was a problem hiding this comment.
Does the scheduler not need reuse between the gl_client and node_client?
There was a problem hiding this comment.
Yes, this indeed can optimize further the startup and save one ssl handshake during startup (as currently we connect twice to the scheduler).
After submiting this: #303
It occurred to me that we can push both connections initializations to the connect (https://github.com/breez/breez-sdk/pull/303/files#diff-7466a53546c6cded460ef5502a9168b13cc930d94fdf7d0ea8166bb10c75fe7eR127) method passing both connections to the Greenlight constructor and get rid of the Mutexes totally.
roeierez
force-pushed
the
gl-connection-reuse
branch
from
47c9752
to
08a838b
Compare
This will reuse the GL gRPC connection and reduce dramatically the number of tls handshakes.
From some tests I ran it shows 4x performance improvments.
This depends on greenlight to deploy their solution to constant URL per node.