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

Reuse notify channel when blocked on read #291

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Reuse notify channel when blocked on read #291

merged 1 commit into from
Apr 28, 2024

Conversation

paulwe
Copy link
Contributor

@paulwe paulwe commented Apr 28, 2024

No description provided.

Copy link

codecov bot commented Apr 28, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 2 lines in your changes are missing coverage. Please review.

Project coverage is 83.19%. Comparing base (f99e891) to head (410cfa5).

Files Patch % Lines
packetio/buffer.go 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #291      +/-   ##
==========================================
- Coverage   83.32%   83.19%   -0.14%     
==========================================
  Files          40       40              
  Lines        2759     2761       +2     
==========================================
- Hits         2299     2297       -2     
- Misses        335      338       +3     
- Partials      125      126       +1     
Flag Coverage Δ
go 82.98% <87.50%> (-0.07%) ⬇️
wasm 65.39% <87.50%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paulwe paulwe requested a review from cnderrauber April 28, 2024 02:23
close(notify)
if waiting {
select {
case b.notify <- struct{}{}:
Copy link
Member

Choose a reason for hiding this comment

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

It will only wake one reading if there are multiple readings blocked and the other will blocked infinite, I believe pion doesn't have this use case now but it would be safer to keep the behavior since it is a public repo.

Copy link
Contributor Author

@paulwe paulwe Apr 28, 2024

Choose a reason for hiding this comment

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

reading concurrently from a buffer doesn't make sense. this isn't a reasonable expectation.

Copy link
Member

Choose a reason for hiding this comment

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

Agree that is not reasonable for the rtp/rtcp packets, just concern if there was other user use it as a normal buffer since it can be used to store any data.

@paulwe paulwe merged commit 64a0285 into master Apr 28, 2024
15 checks passed
@paulwe paulwe deleted the alloc-on-block branch April 28, 2024 03:32
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.

2 participants