add OnOpen() handler and fire it when appropriate

[jwhited]

Description

webrtc.DataChannel.OnOpen() will fire before a datachannel has been ack'd. This PR implements an OnOpen() handler for a datachannel.DataChannel that is invoked when a DATA_CHANNEL_ACK is sent or received. The intention is for this to bubble up to webrtc.DataChannel.OnOpen().

Reference issue

This is the first step towards fixing pion/webrtc#1063.

[jwhited]

@Sean-Der who would be best to review this?

[codecov]

Codecov Report

Merging #56 into master will decrease coverage by 5.56%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
- Coverage   64.41%   58.84%   -5.57%     
==========================================
  Files           4        4              
  Lines         222      243      +21     
==========================================
  Hits          143      143              
- Misses         58       79      +21     
  Partials       21       21              

Flag	Coverage Δ
#go	58.84% <0.00%> (-5.57%)	⬇️
#wasm	58.84% <0.00%> (-5.57%)	⬇️

Impacted Files	Coverage Δ
datachannel.go	57.30% <0.00%> (-7.67%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2ae4a2f...7fe0ff9. Read the comment docs.

[jwhited]

One issue to be considered is that processing of the DCEP ACK (leading to OnOpen handler firing) via handleDCEP() is dependent on a user-controlled read loop:

datachannel/datachannel.go

Lines 186 to 204 in 2ae4a2f

	func (c *DataChannel) ReadDataChannel(p []byte) (int, bool, error) {
	for {
	n, ppi, err := c.stream.ReadSCTP(p)
	if err == io.EOF {
	// When the peer sees that an incoming stream was
	// reset, it also resets its corresponding outgoing stream.
	closeErr := c.stream.Close()
	if closeErr != nil {
	return 0, false, closeErr
	}
	}
	if err != nil {
	return 0, false, err
	}
	var isString bool
	switch ppi {
	case sctp.PayloadTypeWebRTCDCEP:
	err = c.handleDCEP(p[:n])

For typical usage of pion/webrtc.Datachannel this will not be an issue as that package launches a read loop when the DataChannel is created, but may break detached dataChannel implementations where users would likely not start reading until OnOpen() fires:
https://github.com/pion/webrtc/blob/c0032c4d18adcbf3a2325b2ccd0831168b969d2e/datachannel.go#L291-L293

I think we need a separate goroutine to read the initial DCEP messages that datachannel.ReadDataChannel() blocks on.

[Sean-Der]

Oof really sorry @jwhited I am gonna get this merged tomorrow! I am the best person to review :)

Things have been even busier then usual lately

[Sean-Der]

@jwhited sorry about the delay working through this now!

I don't think we should have the if open block, I just checked in the browser and OnOpen events are not fired when setting the handler late.

[Sean-Der]

Beyond this LGTM! Would love to know if there is something I am missing, I can merge and delete this block myself though :)

[jwhited]

Thanks for taking a look!

I carried this over from pion/webrtc.Datachannel which always fires the handler. Removing this would cause the "Server" side to never fire as there's no way to set the handler before Server() returns.

[jwhited]

@Sean-Der would love your thoughts on #56 (comment)

[Sean-Der]

Good catch! It is a shame to add the extra complexity, I am not sure what the right answer is.

I am tempted to call this a breaking change and have a flag OpenFiredOnACK that only pion/webrtc enables. It is nice to add another read loop, but performance sensitive users will notice and be hurt right away. We have the same issue with pion/dtls, it causes a lot of friction and people trying to refactor/rewrite it constantly.

Then we have break things in /v3. Tell users that OnOpen will not fire unless you call Read

[AeroNotix]

@Sean-Der I am needing fixes for this and pion/webrtc#1063 relatively soon. How can I help get these two merged and fixed?

[chrisprobst]

@Sean-Der Any movement on this?

[ashellunts]

Merged in #122