-
Notifications
You must be signed in to change notification settings - Fork 82
Implement pacing interceptor #309
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
base: master
Are you sure you want to change the base?
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #309 +/- ##
==========================================
- Coverage 79.48% 79.30% -0.19%
==========================================
Files 82 84 +2
Lines 4061 4174 +113
==========================================
+ Hits 3228 3310 +82
- Misses 662 687 +25
- Partials 171 177 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
c12a5b4 to
6f6187a
Compare
bfc0780 to
808eb87
Compare
| } | ||
|
|
||
| func (i *Interceptor) loop() { | ||
| ticker := time.NewTicker(i.interval) |
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.
We don't seem to be calling ticker.Stop()
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 added a defer ticker.Stop(), although I don't think we necessarily need it since Go 1.23, because the ticker should be garbage collected when it is unreferenced, even without calling Stop. At least that's how I understand the documentation.
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 read about this, sadly. it's going to be a while before we can drop go 1.22 and 1.21 in pion, also the new behavior is eligible for collection immediately, and it doesn't say that it’s collected immediately, at least from my understanding, but i didn't test it.
Thanks.
808eb87 to
fa8021f
Compare
fa8021f to
8a125e2
Compare
Description
Implements a pacing interceptor separate from congestion control. The interceptor calls back to implementations of the
Pacerinterface whenever a packet is sent.