Repository navigation
Update samples to use RTCPeerConnectionState and show graceful teardown #839
Description
Your environment.
- Version: 2.1.4
- Browser: chrome 76.0.3809.100 (Official Build) (64-bit) / Firefox 69.0
What did you do?
Have a standard audio channel packet handling loop which is basically:
for {
rtpPacket, err := rxtrack.ReadRTP()
... do stuff with rtpPacket
}
Started streaming on audio channel with Chrome. Closed chrome tab. Server now appears to be blocked forever in track.ReadRTP, which is a resource leak of the goroutine.
What did you expect?
Some kind of timeout, or other way to not block? A potentially malicious actor could DDOS the server by opening multiple tracks and not sending any RTP packets. Must the remote side be trusted not to do this?
What happened?
Goroutine calling readRTP appears to block forever.
Reacted by sebin babu
Metadata
Metadata
Assignees
Labels
No labels
Activity
Sean-Der commentedon Sep 19, 2019
Hey @chrbsg, thanks for filing this issue I love feedback from users. Currently this is working as designed (this isn't to say we can't make it better)
ReadRTPandReadwill block until the PeerConnection is closed. I wouldn't be against adding a timeout, we will need to think about the API/make sure we are consistent with putting it everywhere!The remote can't send as many tracks as it wants though, it can only send as many tracks as the
Transceiversyou added. If you were using PlanB it would be possible to DoS someone with unlimited tracks though! But if you are using UnifiedPlan you should be fine.There might not be further action on this, but in that case we should revise documentation/communicate to the user better about this. thanks!
chrbsg commentedon Sep 20, 2019
Thanks for the quick reply. I didn't know that peerconnection.Close would unblock ReadRTP, but it does and it works. I didn't see peerconnection.Close being used in any of the pion examples so I wasn't calling it at all - is it necessary/expected to call this manually, or is the garbage collector eventually going to do the equivalent? I'm now calling it when ICE state changes to disconnected or failed.
Sean-Der commentedon Sep 24, 2019
@chrbsg
Correct we don't ever automatically tear down the PeerConnection.
We could do that in the future though! That would be a breaking change so something we should do for
v3. I believe it would be more confusing to people however.If you are interested I would love a PR that updates the examples to does a
CloseonICEConnectionStateDisconnectedand explains why. That could be really helpful for people in the future![-]track.ReadRTP blocks forever when remote stops sending packets[/-][+]Update samples to use RTCPeerConnectionState and show graceful teardown[/+]Update examples
Update examples
Update examples
Update examples
Update examples
Update examples
Update examples
Update examples
Update examples