You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
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 the handler() closure directly (from the same go-routine), whereas a similar onConnectionStateChange calls the handler() by starting a new go-routine. Both handlers are otherwise identical.
onSignalingStateChange also calls the handler() in its own go-routine (unlike onICEConnectionStateChange).
Certain callbacks are stored as atomic values and are modified and accessed via Store() and Load() methods. While some other callbacks are not atomic and protected by the same peer connection mutex mu as many other things inside of a peer connection.
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:
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: