-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Unify the handling of the peer connection callbacks #2405
Labels
Comments
Thanks for filing this @daniel-abramov I have had users ask about this/be confused. I think this is best for users
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Summary
While reading the code of Pion, I've noticed that peer connection callbacks have almost identical [semantically] code, but they are handled in slightly different ways for no apparent reason. I don't know if there is a strong reason for that, but I could not find any comments, so I assume they could be unified.
Here are some of the things that I noticed:
onICEConnectionStateChange
calls thehandler()
closure directly (from the same go-routine), whereas a similaronConnectionStateChange
calls thehandler()
by starting a new go-routine. Both handlers are otherwise identical.onSignalingStateChange
also calls thehandler()
in its own go-routine (unlikeonICEConnectionStateChange
).Store()
andLoad()
methods. While some other callbacks are not atomic and protected by the same peer connection mutexmu
as many other things inside of a peer connection.webrtc/peerconnection.go
Lines 70 to 75 in 31c8f0a
Motivation
I think it would be easier to unify their handling. This reduces the possibility for new bugs and avoids confusion for new contributors, i.e. if there is no strong reason to handle them in a different way (there is currently no comment about that), then it probably would make sense to unify the code for them.
Additional context
I stumbled across it while examining a seldom dead-lock that I encountered while working with Pion on the backend. If we could achieve unified handling of the callbacks, i.e. make them all atomic (instead of holding and releasing the mutex) and e.g. start them all in a separate go-routine (it's already done like this for the majority of the callbacks from what I can see), the issue could have been mitigated.
The text was updated successfully, but these errors were encountered: