fix: avoid deadlock when replacing blockchain client websocket connection

[dinhani-cw]

After a websocket reconnection happened with the blockchain, Stratus was dead-locking when updating the websocket connection.

Turns out you cannot hold a read lock and try to acquire a write lock in the same Tokio task, so we need to drop the read lock to acquire the write lock and then update the connection.

[github-actions]

PR Review 🔍

⏱️ Estimated effort to review [1-5]	3, because the changes involve concurrency and locking mechanisms which are critical and can be tricky to handle correctly in asynchronous environments like Tokio. Understanding the context and ensuring no deadlocks or race conditions are introduced requires careful analysis.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: The use of unwrap() on self.ws.as_ref() and self.ws_url.as_ref() could lead to a panic if these values are None. This is a risky approach in production code where robustness is critical.
🔒 Security concerns	No

Code feedback:

---

relevant file	src/infra/blockchain_client/blockchain_client.rs
suggestion	Consider handling the potential None case for self.ws.as_ref() and self.ws_url.as_ref() gracefully instead of using unwrap(). This could prevent runtime panics if these values are not properly initialized. [important]
relevant line	let mut ws_write = self.ws.as_ref().unwrap().write().await;

---

relevant file	src/infra/blockchain_client/blockchain_client.rs
suggestion	To enhance error handling, consider logging the error before dropping the read lock ws_read when the websocket connection fails and needs to be recreated. This can provide better debugging information in case of failures. [medium]
relevant line	drop(ws_read);

---

[github-actions]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add error handling for potential None value in self.ws Consider checking if self.ws is None before unwrapping it to avoid potential panics if self.ws is not set. This can be crucial in production environments where stability is key. src/infra/blockchain_client/blockchain_client.rs [278] -let mut ws_write = self.ws.as_ref().unwrap().write().await; +let mut ws_write = self.ws.as_ref().ok_or(anyhow!("WebSocket client not initialized"))?.write().await; Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug that could cause a panic if self.ws is None. Adding error handling for this case improves the stability and robustness of the code, especially in production environments.	9
Performance	Optimize WebSocket read instance creation by moving it outside the loop To avoid repeatedly creating a new WebSocket read instance in each iteration of the loop, consider maintaining a persistent WebSocket read instance outside the loop. This can improve performance by reducing overhead. src/infra/blockchain_client/blockchain_client.rs [255] -let ws_read = self.require_ws().await?; +let mut ws_read = self.require_ws().await?; +loop { + // use ws_read here Suggestion importance[1-10]: 8 Why: This suggestion improves performance by reducing the overhead of repeatedly creating a new WebSocket read instance in each loop iteration. It is a significant optimization that can enhance the efficiency of the code.	8
Best practice	Handle the result of std::mem::replace to manage old resources properly It's a good practice to handle the result of std::mem::replace to ensure that the old WebSocket client is properly dropped or reused, which can prevent resource leaks. src/infra/blockchain_client/blockchain_client.rs [279] -let _ = std::mem::replace(&mut *ws_write, new_ws_client); +if let Some(old_client) = std::mem::replace(&mut *ws_write, new_ws_client) { + drop(old_client); // Explicitly drop the old client +} Suggestion importance[1-10]: 7 Why: Handling the result of std::mem::replace ensures that resources are managed properly, preventing potential resource leaks. This is a best practice that enhances code reliability and maintainability.	7
Maintainability	Clarify the usage scope of ws_read by documenting its lifecycle Using drop explicitly is good for clarity, but consider ensuring that ws_read is not used after this point to prevent accidental misuse which could lead to runtime errors. src/infra/blockchain_client/blockchain_client.rs [277] -drop(ws_read); +drop(ws_read); // Ensure ws_read is not used beyond this point Suggestion importance[1-10]: 6 Why: This suggestion improves code maintainability by clarifying the lifecycle of ws_read. While it is a minor improvement, it helps prevent accidental misuse and potential runtime errors.	6