-
Notifications
You must be signed in to change notification settings - Fork 582
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
chore: Update pion/ice fork to resolve goroutine leak #78
Conversation
Codecov Report
@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 72.02% 72.08% +0.05%
==========================================
Files 88 88
Lines 3421 3417 -4
Branches 55 55
==========================================
- Hits 2464 2463 -1
+ Misses 750 746 -4
- Partials 207 208 +1
Continue to review full report at Codecov.
|
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.
seems sound
@@ -281,25 +274,36 @@ func (c *Conn) negotiate() { | |||
case remoteDescription = <-c.remoteSessionDescriptionChannel: | |||
} | |||
|
|||
c.opts.Logger.Debug(context.Background(), "setting remote description") |
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.
you should put fields on these so they have more details
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.
Didn't wanna leak the session description in logs, since it technically contains the exchange keys.
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.
Is this a risk? We already log this stuff on coder already. Only the two peers would be able to see the keys.
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.
Doesn't really help IMO. Since we catch all errors, this would error if the session description was invalid.
@@ -2,9 +2,6 @@ module github.com/coder/coder | |||
|
|||
go 1.17 | |||
|
|||
// Required until https://github.com/pion/ice/pull/413 is merged. | |||
replace github.com/pion/ice/v2 => github.com/kylecarbs/ice/v2 v2.1.8-0.20220127013758-526c25708344 |
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.
nice, good to get rid of that fork 👍
No description provided.