race when simulcast

[mission-liao]

Your environment.

• Version: v3.0.28
• Browser: chrome v90

What did you do?

• create a PeerConnection as offer side in browser, and setup for simulcast
• create a PeerConnection as answer side in pion, and start reading RTCP packets upon onTrack events

What did you expect?

• it should not race
• I've investigated code around the race, however, these parts are performance sensitive. Need more input before modifying them.

What happened?

the stack trace when race detected

WARNING: DATA RACE
Write at 0x00c000d260b8 by goroutine 152:
  github.com/pion/webrtc/v3.(*RTPReceiver).receiveForRid()
      /vendor/github.com/pion/webrtc/v3/rtpreceiver.go:281 +0x73a
  github.com/pion/webrtc/v3.(*PeerConnection).handleUndeclaredSSRC()
      /vendor/github.com/pion/webrtc/v3/peerconnection.go:1423 +0xb8f
  github.com/pion/webrtc/v3.(*PeerConnection).undeclaredMediaProcessor.func1.1()
      /vendor/github.com/pion/webrtc/v3/peerconnection.go:1468 +0x97

Previous read at 0x00c000d260b8 by goroutine 149:
  github.com/pion/webrtc/v3.(*RTPReceiver).ReadSimulcast()
      /vendor/github.com/pion/webrtc/v3/rtpreceiver.go:165 +0x1a7
  github.com/pion/webrtc/v3.(*RTPReceiver).ReadSimulcastRTCP()
      /vendor/github.com/pion/webrtc/v3/rtpreceiver.go:196 +0xb5

Goroutine 152 (running) created at:
  github.com/pion/webrtc/v3.(*PeerConnection).undeclaredMediaProcessor.func1()
      /vendor/github.com/pion/webrtc/v3/peerconnection.go:1465 +0x3cc

Goroutine 149 (running) created at:

[mission-liao]

@Sean-Der Looking for your opinion to fix this problem, I could help to submit a PR with e2e test during holidays.

It seems the simplest way to avoid race is adding a mutex in RTPReceiver and locking when ReadSimulcast and receiveForRid. But I'm not so sure about it.

[jech]

There already is a mutex, and it's taken in receiveForRid. The issue is that the mutex is not taken in ReadSimulcast.
Essentially, the current code assumes that the set of tracks does not change during Read, which is no longer true with simulcast.

Not sure if it's correct to take the mutex in ReadSimulcast, since if it's ever taken in interceptor.Read, we would deadlock.

[mission-liao]

Oh, I didn't notice that there is already a mutex, thanks.

Hmm, it seems not reasonable to lock when Read(Simulcast). I think the best solution I could image is applying the copy-on-write pattern here:

• RTPReceiver.tracks is stored in an atomic.Value.
• in Read(Simulcast), load RTPReceiver.tracks without locking
• in receiveForRid, deep-copy and modify RTPReceiver.tracks, then store them back to atomic with locking. Although it looks expensive, but I think the frequency of write is not high.

However,

• that would introduce some performance penalty when read.
• RTPReceiver.tracks is accessed everywhere ... it takes time to review all related scenarios.