pion / webrtc

Pure Go implementation of the WebRTC API
https://pion.ly
MIT License
13.79k stars 1.66k forks source link

Fix data-channels-flow-control example #846

Open hugoArregui opened 5 years ago

hugoArregui commented 5 years ago

There is a problem with the current example, the peer may stop sending data here if the max buffer size has not been reached before this point and since the callback is executed as part of the stream processing thread, that will block everything.

So we need to:

1) Find a better flow control mechanism for the example 2) Decide if we should execute the callback in another goroutine so we ensure we don't block the stream, or, document the fact that users should make sure their code don't block.

As a suggestion for (1) I wrote this code: https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L31 which uses a queue and drains it whenever it's possible. It might not the most efficient way having a channel in the middle of the write operation but I like how easy is to reason about it. I think it makes sense for an example.

Regarding (2) I think we should probably execute the callback in another goroutine since that's consistent with do things in other places, but I have no strong opinion on this matter.

hugoArregui commented 5 years ago

CC @enobufs

AeroNotix commented 5 years ago

How have you tested the example (suggestion 1)? I have tried it myself and found a few issues. I am still isolating my own code from that example but I would like to see how you tested it to verify for correctness?

hugoArregui commented 5 years ago

@AeroNotix I ran it with a 1.5gb file (uncommenting https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L138 and adding a loop when I wanted more data) and different combinations of max buffer size / queue size / threshold. In this particular case please notice the channel is unreliable (https://github.com/pion/webrtc/blob/topic-test-dc-perf/examples/data-channels-flow-control/main.go#L120), so if you want to try this very same example you may want to set some max retransmits.

AeroNotix commented 5 years ago

It would be interesting to have a runnable example, alone, that you specify what parameters it is supposed to exhibit - then I run that - and I can confirm or deny that it exhibits the same or similar performance.

Without that we are pissing into the dark, with the wind at our face.

AeroNotix commented 5 years ago

E.g. I would be interested in these metrics, from your example: