From: Matt Corallo <git@bluematt.me>
Date: Sun, 21 Jul 2024 23:29:19 +0000 (+0000)
Subject: [block-sync] Don't hold client-cache lock while connecting
X-Git-Tag: v0.0.124-beta~38^2
X-Git-Url: http://git.bitcoin.ninja/index.cgi?a=commitdiff_plain;h=refs%2Fheads%2F2024-07-stupid-locked-connect;p=rust-lightning

[block-sync] Don't hold client-cache lock while connecting

`lightning-block-sync`'s REST and RPC clients both hold a cache for
a connected client to avoid the extra connection round-trip on each
request. Because only one client can be using a connection at once,
the connection is `take()`n out of an `Option` behind a `Mutex` and
if there isn't one present, we call `HttpClient::connect` to build
a new one.

However, this full logic is completed in one statement, causing a
client-cache lock to be held during `HttpClient::connect`. This
can turn into quite a bit of contention when using these clients as
gossip verifiers as we can create many requests back-to-back during
startup.

I noticed this as my node during startup only seemed to be
saturating one core and managed to get a backtrace that showed
several threads being blocked on this mutex when hitting a Bitcoin
Core node over REST that is on the same LAN, but not the same
machine.
---

diff --git a/lightning-block-sync/src/rest.rs b/lightning-block-sync/src/rest.rs
index 9473f7b1b..74837f08f 100644
--- a/lightning-block-sync/src/rest.rs
+++ b/lightning-block-sync/src/rest.rs
@@ -34,7 +34,8 @@ impl RestClient {
 	{
 		let host = format!("{}:{}", self.endpoint.host(), self.endpoint.port());
 		let uri = format!("{}/{}", self.endpoint.path().trim_end_matches("/"), resource_path);
-		let mut client = if let Some(client) = self.client.lock().unwrap().take() {
+		let reserved_client = self.client.lock().unwrap().take();
+		let mut client = if let Some(client) = reserved_client {
 			client
 		} else {
 			HttpClient::connect(&self.endpoint)?
diff --git a/lightning-block-sync/src/rpc.rs b/lightning-block-sync/src/rpc.rs
index 8032d3fcc..59c7e589e 100644
--- a/lightning-block-sync/src/rpc.rs
+++ b/lightning-block-sync/src/rpc.rs
@@ -77,7 +77,8 @@ impl RpcClient {
 			"id": &self.id.fetch_add(1, Ordering::AcqRel).to_string()
 		});
 
-		let mut client = if let Some(client) = self.client.lock().unwrap().take() {
+		let reserved_client = self.client.lock().unwrap().take();
+		let mut client = if let Some(client) = reserved_client {
 			client
 		} else {
 			HttpClient::connect(&self.endpoint)?