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.

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-2268 - Update webrtc version #32

Merged
merged 5 commits into from
Mar 14, 2023

Conversation

stuqdog
Copy link
Member

@stuqdog stuqdog commented Mar 13, 2023

Updates us to webrtc v0.7.1, creates a blocking close fn for WebRTCBaseChannel to facilitate the upgrade.

@stuqdog stuqdog requested a review from npmenard March 13, 2023 18:53
@stuqdog stuqdog requested a review from a team as a code owner March 13, 2023 18:53
@@ -72,6 +72,60 @@ impl RPCCredentials {
}
}

impl ViamChannel {
async fn create_resp(
Copy link
Member Author

Choose a reason for hiding this comment

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

note to reviewers: this is big chunk of code but it's basically just factoring out some code into a helper function. Shouldn't need too much look.

@stuqdog stuqdog requested a review from benjirewis March 13, 2023 18:57
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Fixes the integer overflow on my machine. Just a couple comments.

pub(crate) fn new_stream(&self) -> Result<Stream> {
// 256 is an arbitrarily high number for maximum concurrent streams, determined based on
// analogous value in goutils
if self.streams.len() >= 256 {
Copy link
Member

Choose a reason for hiding this comment

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

[nit] parameterize 256.

src/rpc/dial.rs Outdated
Err(e) => {
log::error!("{e}");
let response = response
.header("grpc-status", &STATUS_CODE_UNKNOWN.to_string())
Copy link
Member

Choose a reason for hiding this comment

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

[opt] a more informative error code would be RESOURCE_EXHAUSTED. Returning that here would assume that new_stream never returns any other kind of error, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

ooh yeah thanks for the reminder on that one! fixed :)

};
match channel.new_stream() {
Err(e) => {
log::error!("{e}");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
log::error!("{e}");
log::error!("error getting new stream: {e}");

Copy link
Member Author

Choose a reason for hiding this comment

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

I opted for just printing the error here because I think the error message from new_stream() is already pretty descriptive, so adding the prefix there just makes it feel wordy. Happy to chat more if you disagree :)

Copy link
Member

Choose a reason for hiding this comment

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

No need! Makes sense to me.

Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

Thanks 🧑‍🔧 !

Copy link
Member

@npmenard npmenard left a comment

Choose a reason for hiding this comment

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

LGTM

@stuqdog stuqdog merged commit 0da4b35 into viamrobotics:main Mar 14, 2023
@stuqdog stuqdog deleted the update-webrtc-version branch March 14, 2023 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants