Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(eth-websocket): remove some unnecessary wrappers #2291

Merged
merged 5 commits into from
Dec 26, 2024
Merged

chore(eth-websocket): remove some unnecessary wrappers #2291

Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
3798a29
remove unnecessary Arc<
mariocynicys Jun 17, 2024
a394c6a
eth websocket: avoid locking control message sender and clone it instead
mariocynicys Jun 17, 2024
f16b564
merge with origin/dev
mariocynicys Dec 13, 2024
6eb6fc3
review(sami): move tx declaration close to where it's being used
mariocynicys Dec 25, 2024
8ae149b
review(sami): free rx end out of the Arc
mariocynicys Dec 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 7 additions & 8 deletions mm2src/coins/eth/web3_transport/websocket_transport.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,13 @@ pub struct WebsocketTransport {
node: WebsocketTransportNode,
event_handlers: Vec<RpcTransportEventHandlerShared>,
pub(crate) proxy_sign_keypair: Option<Keypair>,
controller_channel: Arc<ControllerChannel>,
controller_channel: ControllerChannel,
connection_guard: Arc<AsyncMutex<()>>,
}

#[derive(Debug)]
#[derive(Clone, Debug)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why replacing Arc with Clone?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

both tx & rx inside were Arced which made me think we extract/clone them out of ContollerChannel so wrapping with a bigger arc is just a complex optimization.
this doesn't seem the case though (tx already clonable, rx was never cloned out in the first place).
8ae149b

struct ControllerChannel {
tx: Arc<AsyncMutex<UnboundedSender<ControllerMessage>>>,
tx: UnboundedSender<ControllerMessage>,
rx: Arc<AsyncMutex<UnboundedReceiver<ControllerMessage>>>,
}
borngraced marked this conversation as resolved.
Show resolved Hide resolved

Expand Down Expand Up @@ -87,10 +87,9 @@ impl WebsocketTransport {
event_handlers,
request_id: Arc::new(AtomicUsize::new(1)),
controller_channel: ControllerChannel {
tx: Arc::new(AsyncMutex::new(req_tx)),
tx: req_tx,
rx: Arc::new(AsyncMutex::new(req_rx)),
}
.into(),
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

rx: AsyncMutex::new(req_rx)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that arc was indeed the unnecessary one, thanks :)
8ae149b

connection_guard: Arc::new(AsyncMutex::new(())),
proxy_sign_keypair: None,
last_request_failed: Arc::new(AtomicBool::new(false)),
Expand Down Expand Up @@ -298,7 +297,7 @@ impl WebsocketTransport {
}

pub(crate) async fn stop_connection_loop(&self) {
let mut tx = self.controller_channel.tx.lock().await;
let mut tx = self.controller_channel.tx.clone();
tx.send(ControllerMessage::Close)
.await
.expect("receiver channel must be alive");
Expand Down Expand Up @@ -357,7 +356,7 @@ async fn send_request(
serialized_request = serde_json::to_string(&wrapper)?;
}

let mut tx = transport.controller_channel.tx.lock().await;
let mut tx = transport.controller_channel.tx.clone();
borngraced marked this conversation as resolved.
Show resolved Hide resolved

let (notification_sender, notification_receiver) = oneshot::channel::<Vec<u8>>();

Expand Down
Loading