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

Remove buffer waiting condition #305

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Conversation

edaniels
Copy link
Member

@edaniels edaniels commented Jul 23, 2024

Buffer's usage of waiting causes a scenario where if two reads come in while a write is happening can cause waiting to be stuck in false and all write "signals" will be dropped. After reading through the code, I can't figure out why the waiting condition is necessary; the worst thing I can imagine is we don't send to the buffered channel as often.

Fixes #299

@edaniels edaniels requested a review from paulwe July 23, 2024 02:18
@edaniels
Copy link
Member Author

@paulwe thanks for the feedback on #304. After reading through my code, I realized removing waiting was what mattered for buffer's specific use case.

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.20%. Comparing base (d55a60c) to head (7fee991).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #305      +/-   ##
==========================================
+ Coverage   83.08%   83.20%   +0.11%     
==========================================
  Files          39       39              
  Lines        2726     2721       -5     
==========================================
- Hits         2265     2264       -1     
+ Misses        335      332       -3     
+ Partials      126      125       -1     
Flag Coverage Δ
go 83.11% <100.00%> (+0.19%) ⬆️
wasm 65.07% <100.00%> (-0.03%) ⬇️

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.

@edaniels
Copy link
Member Author

Confirmed with this change it fixes #299 when running go test -run TestPeerConnection_Simulcast_Probe/Break_NonSimulcast -count=100. Without it, it fails on the first run.

Copy link
Contributor

@paulwe paulwe left a comment

Choose a reason for hiding this comment

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

the goal was to reduce the overhead from writing when there is no pending read but the state gets mangled if there is more than one reader. i mistakenly assumed there was no use case for concurrent reading. it's probably possible to implement this with a counter instead of a bool but it's not critical...

@edaniels
Copy link
Member Author

the goal was to reduce the overhead from writing when there is no pending read but the state gets mangled if there is more than one reader. i mistakenly assumed there was no use case for concurrent reading. it's probably possible to implement this with a counter instead of a bool but it's not critical...

I'll give the counter some short thought!

@edaniels edaniels merged commit a0000b2 into pion:master Jul 23, 2024
14 checks passed
@edaniels edaniels deleted the no_waiting_bool branch July 23, 2024 03:00
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.

Regression in v2.2.5 - pion/webrtc simulcast tests fail
2 participants