From c10e5c10d67954ce322c4f25a255a1ce8a278972 Mon Sep 17 00:00:00 2001
From: Ethan <ethan@viam.com>
Date: Wed, 5 Apr 2023 14:30:40 +0000
Subject: [PATCH] RSDK-2407 - Fix gRPC error logging (#40)

---
 src/rpc/base_channel.rs   | 32 +++++++-------------------------
 src/rpc/client_channel.rs |  6 ------
 src/rpc/client_stream.rs  | 15 ++++++---------
 3 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/src/rpc/base_channel.rs b/src/rpc/base_channel.rs
index 90fd1ee..d58617f 100644
--- a/src/rpc/base_channel.rs
+++ b/src/rpc/base_channel.rs
@@ -1,7 +1,6 @@
 use anyhow::Result;
 use std::{
     fmt::Debug,
-    panic::{catch_unwind, set_hook, AssertUnwindSafe},
     sync::{
         atomic::{AtomicBool, AtomicPtr, Ordering},
         Arc,
@@ -27,6 +26,12 @@ impl Debug for WebRTCBaseChannel {
     }
 }
 
+impl Drop for WebRTCBaseChannel {
+    fn drop(&mut self) {
+        log::debug!("Dropping base channel {self:?}");
+    }
+}
+
 impl WebRTCBaseChannel {
     pub(crate) async fn new(
         peer_connection: Arc<RTCPeerConnection>,
@@ -62,6 +67,7 @@ impl WebRTCBaseChannel {
 
         let c = Arc::downgrade(&channel);
         dc.on_error(Box::new(move |err: webrtc::Error| {
+            log::error!("Data channel error: {err}");
             let c = match c.upgrade() {
                 Some(c) => c,
                 None => return Box::pin(async {}),
@@ -69,9 +75,6 @@ impl WebRTCBaseChannel {
             Box::pin(async move {
                 let mut err = Some(anyhow::Error::from(err));
                 c.closed_reason.store(&mut err, Ordering::Release);
-                if let Err(e) = c.close_sync() {
-                    log::error!("error closing channel: {e}")
-                }
             })
         }));
 
@@ -93,27 +96,6 @@ impl WebRTCBaseChannel {
             .map_err(anyhow::Error::from)
     }
 
-    // `close` with blocking. Should only be used in contexts where an async close is disallowed
-    pub(crate) fn close_sync(&self) -> Result<()> {
-        set_hook(Box::new(|info| {
-            log::error!(
-        "Unable to close base_channel gracefully. This may be because of an attempt to connect to a robot that isn't online. Error message: {}",
-        info.to_string()
-        )
-        }));
-        let safe_self = AssertUnwindSafe(self);
-        match catch_unwind(|| {
-            let rt = tokio::runtime::Builder::new_current_thread()
-                .enable_all()
-                .build()
-                .unwrap();
-            rt.block_on(async move { safe_self.close().await })
-        }) {
-            Ok(res) => res,
-            Err(_) => Err(anyhow::anyhow!("Unable to close base channel gracefully")),
-        }
-    }
-
     /// Returns whether or not the channel is closed
     #[allow(dead_code)]
     pub fn is_closed(&self) -> bool {
diff --git a/src/rpc/client_channel.rs b/src/rpc/client_channel.rs
index 80fc8c8..52ad13a 100644
--- a/src/rpc/client_channel.rs
+++ b/src/rpc/client_channel.rs
@@ -46,12 +46,6 @@ impl Debug for WebRTCClientChannel {
 
 impl Drop for WebRTCClientChannel {
     fn drop(&mut self) {
-        let bc = self.base_channel.clone();
-        if !bc.is_closed() {
-            if let Err(e) = bc.close_sync() {
-                log::error!("Error closing base channel: {e}")
-            }
-        };
         log::debug!("Dropping client channel {:?}", &self);
     }
 }
diff --git a/src/rpc/client_stream.rs b/src/rpc/client_stream.rs
index c3b1d9f..aacaf4b 100644
--- a/src/rpc/client_stream.rs
+++ b/src/rpc/client_stream.rs
@@ -44,7 +44,7 @@ impl WebRTCClientStream {
         Ok(())
     }
 
-    async fn process_trailers(&mut self, trailers: ResponseTrailers) -> Result<()> {
+    async fn process_trailers(&mut self, trailers: ResponseTrailers) {
         let trailers_to_send = trailers_from_proto(trailers.clone());
         if let Err(e) = self
             .base_stream
@@ -69,14 +69,11 @@ impl WebRTCClientStream {
             }
         };
 
-        self.base_stream.close_with_recv_error(&mut err.as_ref());
-        match err {
-            None => Ok(()),
-            Some(err) => {
-                log::error!("Error processing trailers: {err}");
-                Err(err)
-            }
+        if let Some(e) = &err {
+            log::debug!("received gRPC error: {e}");
         }
+
+        self.base_stream.close_with_recv_error(&mut err.as_ref())
     }
 
     // processes response.
@@ -114,7 +111,7 @@ impl WebRTCClientStream {
                 self.process_message(message.to_owned()).await
             }
 
-            Some(Type::Trailers(trailers)) => self.process_trailers(trailers.to_owned()).await,
+            Some(Type::Trailers(trailers)) => Ok(self.process_trailers(trailers.to_owned()).await),
             None => Ok(()),
         }
     }