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

Update samples to use RTCPeerConnectionState and show graceful teardown #839

Closed
chrbsg opened this issue Sep 19, 2019 · 3 comments · Fixed by #1856
Closed

Update samples to use RTCPeerConnectionState and show graceful teardown #839

chrbsg opened this issue Sep 19, 2019 · 3 comments · Fixed by #1856
Assignees

Comments

@chrbsg
Copy link

chrbsg commented Sep 19, 2019

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.

@Sean-Der Sean-Der self-assigned this Sep 19, 2019
@Sean-Der
Copy link
Member

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)

ReadRTP and Read will 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 Transceivers you 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
Copy link
Author

chrbsg commented 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
Copy link
Member

@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 Close on ICEConnectionStateDisconnected and explains why. That could be really helpful for people in the future!

@Sean-Der Sean-Der changed the title track.ReadRTP blocks forever when remote stops sending packets Update samples to use RTCPeerConnectionState and show graceful teardown Sep 24, 2019
Antonito added a commit that referenced this issue Jun 28, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Antonito added a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
Sean-Der pushed a commit that referenced this issue Jul 2, 2021
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes #839
daonb pushed a commit to daonb/webrtc that referenced this issue Nov 20, 2022
TestNonFatalRead now has an timeout.
Examples now use Mime types, instead of raw strings.

Fixes pion#839
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants