-
Notifications
You must be signed in to change notification settings - Fork 24
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.
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
RSDK-2407 - Fix gRPC error logging #40
Conversation
src/rpc/client_stream.rs
Outdated
Err(err) | ||
} | ||
if let Some(e) = &err { | ||
log::error!("received gRPC error: {e}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went with error
level logging here just to increase visibility and because it is definitely an error. However, this may be overkill since we'll still be getting an error from call
? Happy to update to debug
level if people think that's preferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say debug!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
debug it is!
Note to reviewers: build is currently failing due to an upstream issue, being tracked/hopefully resolved here. |
fn drop(&mut self) { | ||
log::debug!("Dropping base channel {self:?}"); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm a little confused by this logic - where do we actually drop the channel object? or is the whole point of this PR that it's happening elsewhere so we should only log here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop()
is what's called when something leaves scope. So if this calls that means the base_channel
is definitely dropped. The log just provides verification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Major changes:
call
function, but this avoids confusing errors that imply that any gRPC call error is a rust deserialization problem)close_sync
- this function was failing before because of issues creating runtimes within runtimes, which caused noisy and erroneous error messages to log when shutting downWebRTCBaseChannel
to show that - despite removing theclose_sync
- we are in fact successfully droppingWebRTCBaseChannel
s when done with them.