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

Make OnConnectionStateChange wait for handler to finish #2702

Closed
wants to merge 4 commits into from

Conversation

sukunrt
Copy link
Member

@sukunrt sukunrt commented Mar 13, 2024

OnConnectionStateChange is used to determine whether the peer connection has succeeded and track connection state by users. It should be called in sync. Right now state changes might get reordered which would lead to tracking errors for users.

Copy link

codecov bot commented Mar 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 76.38%. Comparing base (3f6d94a) to head (cbb2ae3).
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2702      +/-   ##
==========================================
- Coverage   76.42%   76.38%   -0.04%     
==========================================
  Files          87       87              
  Lines        9934     9927       -7     
==========================================
- Hits         7592     7583       -9     
- Misses       1870     1871       +1     
- Partials      472      473       +1     
Flag Coverage Δ
go 77.91% <100.00%> (-0.04%) ⬇️
wasm 64.57% <ø> (-0.09%) ⬇️

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.

peerconnection.go Outdated Show resolved Hide resolved
peerconnection.go Outdated Show resolved Hide resolved
@sukunrt sukunrt force-pushed the connection-update-callback-sync branch from 98dca89 to 9b21064 Compare March 14, 2024 04:12
@sukunrt
Copy link
Member Author

sukunrt commented Mar 14, 2024

I have moved the updates to a operations queue. That'll run the notifications sequentially. This ensures that users can do things like pc.Close from the call back.

@sukunrt sukunrt force-pushed the connection-update-callback-sync branch from 16a9882 to c95df47 Compare March 14, 2024 08:58
@sukunrt sukunrt requested a review from Sean-Der March 14, 2024 13:15
@sukunrt sukunrt force-pushed the connection-update-callback-sync branch from c95df47 to 83ecdaa Compare March 14, 2024 13:21
@Sean-Der
Copy link
Member

This was fixed by pion/ice@67cc918, going to close here!

Will be tagging a new version of pion/ice today! thank you @sukunrt :)

@Sean-Der Sean-Der closed this Mar 20, 2024
@Sean-Der Sean-Der deleted the connection-update-callback-sync branch March 20, 2024 19:03
@sukunrt
Copy link
Member Author

sukunrt commented Jun 8, 2024

It's not exactly fixed. The state reported on the callback isn't reliable. Users need to ignore the state provided on the call back and call pc.ConnectionState() for the accurate state of the connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants