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

Unify the handling of the peer connection callbacks #2405

Open
daniel-abramov opened this issue Feb 6, 2023 · 1 comment
Open

Unify the handling of the peer connection callbacks #2405

daniel-abramov opened this issue Feb 6, 2023 · 1 comment

Comments

@daniel-abramov
Copy link

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 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.

webrtc/peerconnection.go

Lines 70 to 75 in 31c8f0a

onSignalingStateChangeHandler func(SignalingState)
onICEConnectionStateChangeHandler atomic.Value // func(ICEConnectionState)
onConnectionStateChangeHandler atomic.Value // func(PeerConnectionState)
onTrackHandler func(*TrackRemote, *RTPReceiver)
onDataChannelHandler func(*DataChannel)
onNegotiationNeededHandler atomic.Value // func()

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.

@Sean-Der
Copy link
Member

Sean-Der commented May 9, 2024

Thanks for filing this @daniel-abramov I have had users ask about this/be confused. I think this is best for users

  • A user shouldn't be able to cause a deadlock by calling external APIs
  • In cases where high performance matters (like OnMessage for DataChannels) we might have to accept the sharp API
  • I am going to go through and add unit tests that ensure you can call Close in every callback! Will fix the places I find that have issues.

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

No branches or pull requests

2 participants