Skip to content

Commit

Permalink
Address pr feedback paritytech#1
Browse files Browse the repository at this point in the history
  • Loading branch information
billylindeman committed May 10, 2021
1 parent d017af0 commit cb243a4
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 11 deletions.
7 changes: 3 additions & 4 deletions types/src/client.rs
Expand Up @@ -138,9 +138,8 @@ impl<Notif> Drop for Subscription<Notif> {
impl<Notif> Drop for NotificationHandler<Notif> {
fn drop(&mut self) {
// We can't actually guarantee that this goes through. If the background task is busy, then
// the channel's buffer will be full, and our unsubscription request will never make it.
// However, when a notification arrives, the background task will realize that the channel
// to the `Subscription` has been closed, and will perform the unsubscribe.
let _ = self.to_back.send(FrontToBack::UnregisterNotification((&self.method).to_owned())).now_or_never();
// the channel's buffer will be full, and our unregister request will never make it.
let notif_method = std::mem::take(&mut self.method);
let _ = self.to_back.send(FrontToBack::UnregisterNotification(notif_method)).now_or_never();
}
}
8 changes: 3 additions & 5 deletions ws-client/src/client.rs
Expand Up @@ -595,18 +595,16 @@ async fn background_task(
let (subscribe_tx, subscribe_rx) = mpsc::channel(max_notifs_per_subscription);

if manager.insert_notification_handler(&reg.method, subscribe_tx).is_ok() {
reg.send_back
.send(Ok((subscribe_rx, reg.method)))
.expect("error sending response for notification handler");
let _ = reg.send_back.send(Ok((subscribe_rx, reg.method)));
} else {
let _ = reg.send_back.send(Err(Error::MethodAlreadyRegistered(reg.method)));
}
}

// User called `on_notification` on the front-end.
// User dopped the notificationHandler for this method
Either::Left((Some(FrontToBack::UnregisterNotification(method)), _)) => {
log::trace!("[backend] unregistering notification handler: {:?}", method);
let _ = manager.remove_notification_handler(&method);
let _ = manager.remove_notification_handler(method);
}
Either::Right((Some(Ok(raw)), _)) => {
// Single response to a request.
Expand Down
4 changes: 2 additions & 2 deletions ws-client/src/manager.rs
Expand Up @@ -159,8 +159,8 @@ impl RequestManager {
}

/// Removes a notification handler
pub fn remove_notification_handler(&mut self, method: &str) -> Result<(), Error> {
if let Some(_) = self.notification_handlers.remove(&method.to_owned()) {
pub fn remove_notification_handler(&mut self, method: String) -> Result<(), Error> {
if let Some(_) = self.notification_handlers.remove(&method) {
Ok(())
} else {
Err(Error::UnregisteredNotification(method.to_owned()))
Expand Down

0 comments on commit cb243a4

Please sign in to comment.