Unify the handling of the peer connection callbacks

[daniel-abramov]

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]

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.