Repository: pion/webrtc
Pull request #2392 (2023-01-30 01:08:25)

Add OnDial handler for datachannels
Adds an `OnDial` handler to be fired once the dialing side has sent the DCEP OPEN message over the data channel.

DCEP specifies that ordered data can be sent by the dialing side as soon as it has sent the open message ([link](https://www.rfc-editor.org/rfc/rfc8832.html#section-6)):

> After the DATA_CHANNEL_OPEN message has been sent, the sender of that message MAY start sending messages containing user data without waiting for the reception of the corresponding DATA_CHANNEL_ACK message. However, before the DATA_CHANNEL_ACK message or any other message has been received on a data channel, all other messages containing user data and belonging to this data channel MUST be sent ordered, no matter whether the data channel is ordered or not

Leveraging this is useful in applications that only use reliable, ordered delivery, those that want to start sending ordered messages on unordered channels before receiving an ACK back from the other peer, etc. This was possible before https://github.com/pion/webrtc/pull/1994, but was no longer possible after `OnOpen` was (correctly) updated to wait for the ACK to be received.

Follows up on: https://github.com/pion/webrtc/issues/1063

codecov[bot]: # [Codecov](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) Report
Base: **77.60**% // Head: **79.20**% // Increases project coverage by **`+1.60%`** :tada:
> Coverage data is based on head [(`bb7eed9`)](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) compared to base [(`5b41ed6`)](https://codecov.io/gh/pion/webrtc/commit/5b41ed600e00e41d8cebe37d48d9a7fd9f90f669?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).
> Patch coverage: 79.16% of modified lines in pull request are covered.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##           master    #2392      +/-   ##
==========================================
+ Coverage   77.60%   79.20%   +1.60%     
==========================================
  Files          87       76      -11     
  Lines        9275     8402     -873     
==========================================
- Hits         7198     6655     -543     
+ Misses       1643     1354     -289     
+ Partials      434      393      -41     
```

| Flag | Coverage Δ | |
|---|---|---|
| go | `79.20% <79.16%> (-0.18%)` | :arrow_down: |
| wasm | `?` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | Coverage Δ | |
|---|---|---|
| [datachannel.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-ZGF0YWNoYW5uZWwuZ28=) | `84.71% <79.16%> (-0.37%)` | :arrow_down: |
| [internal/mux/mux.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aW50ZXJuYWwvbXV4L211eC5nbw==) | `84.78% <0.00%> (-4.35%)` | :arrow_down: |
| [sctptransport.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2N0cHRyYW5zcG9ydC5nbw==) | `77.15% <0.00%> (-3.38%)` | :arrow_down: |
| [icetransport.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-aWNldHJhbnNwb3J0Lmdv) | `68.42% <0.00%> (-1.22%)` | :arrow_down: |
| [datachannel\_js.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-ZGF0YWNoYW5uZWxfanMuZ28=) | | |
| [sctptransport\_js.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-c2N0cHRyYW5zcG9ydF9qcy5nbw==) | | |
| [js\_utils.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-anNfdXRpbHMuZ28=) | | |
| [api\_js.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-YXBpX2pzLmdv) | | |
| [peerconnection\_js.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cGVlcmNvbm5lY3Rpb25fanMuZ28=) | | |
| [rtptransceiver\_js.go](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion#diff-cnRwdHJhbnNjZWl2ZXJfanMuZ28=) | | |
| ... and [6 more](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion) | |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/pion/webrtc/pull/2392?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=pion).

treyhakanson: Thanks for the review Benny! Will address these comments today
treyhakanson: Addressed initial comments! For some reason re-requesting a review removed Sean, feel free to add him back if you think its necessary

Patch:
diff --git a/datachannel.go b/datachannel.go
index 94e4ddf5..6e41fdc1 100644
--- a/datachannel.go
+++ b/datachannel.go
@@ -49,6 +49,8 @@ type DataChannel struct {
 	onMessageHandler    func(DataChannelMessage)
 	openHandlerOnce     sync.Once
 	onOpenHandler       func()
+	dialHandlerOnce     sync.Once
+	onDialHandler       func()
 	onCloseHandler      func()
 	onBufferedAmountLow func()
 	onErrorHandler      func(error)
@@ -175,6 +177,7 @@ func (d *DataChannel) open(sctpTransport *SCTPTransport) error {
 	dc.OnBufferedAmountLow(d.onBufferedAmountLow)
 	d.mu.Unlock()
 
+	d.onDial()
 	d.handleOpen(dc, false, d.negotiated)
 	return nil
 }
@@ -228,6 +231,30 @@ func (d *DataChannel) onOpen() {
 	}
 }
 
+// OnDial sets an event handler which is invoked when the
+// peer has been dialed, but before said peer has responsed
+func (d *DataChannel) OnDial(f func()) {
+	d.mu.Lock()
+	d.dialHandlerOnce = sync.Once{}
+	d.onDialHandler = f
+	d.mu.Unlock()
+
+	if d.ReadyState() == DataChannelStateOpen {
+		// If the data channel is already open, call the handler immediately.
+		go d.dialHandlerOnce.Do(f)
+	}
+}
+
+func (d *DataChannel) onDial() {
+	d.mu.RLock()
+	handler := d.onDialHandler
+	d.mu.RUnlock()
+
+	if handler != nil {
+		go d.dialHandlerOnce.Do(handler)
+	}
+}
+
 // OnClose sets an event handler which is invoked when
 // the underlying data transport has been closed.
 func (d *DataChannel) OnClose(f func()) {


Test patch:
diff --git a/datachannel_go_test.go b/datachannel_go_test.go
index 126e2eb5..a93ca288 100644
--- a/datachannel_go_test.go
+++ b/datachannel_go_test.go
@@ -33,12 +33,17 @@ func TestDataChannel_EventHandlers(t *testing.T) {
 	api := NewAPI()
 	dc := &DataChannel{api: api}
 
+	onDialCalled := make(chan struct{})
 	onOpenCalled := make(chan struct{})
 	onMessageCalled := make(chan struct{})
 
 	// Verify that the noop case works
 	assert.NotPanics(t, func() { dc.onOpen() })
 
+	dc.OnDial(func() {
+		close(onDialCalled)
+	})
+
 	dc.OnOpen(func() {
 		close(onOpenCalled)
 	})
@@ -48,10 +53,12 @@ func TestDataChannel_EventHandlers(t *testing.T) {
 	})
 
 	// Verify that the set handlers are called
+	assert.NotPanics(t, func() { dc.onDial() })
 	assert.NotPanics(t, func() { dc.onOpen() })
 	assert.NotPanics(t, func() { dc.onMessage(DataChannelMessage{Data: []byte("o hai")}) })
 
 	// Wait for all handlers to be called
+	<-onDialCalled
 	<-onOpenCalled
 	<-onMessageCalled
 }
@@ -578,3 +585,74 @@ func TestDataChannel_NonStandardSessionDescription(t *testing.T) {
 	<-onDataChannelCalled
 	closePairNow(t, offerPC, answerPC)
 }
+
+func TestDataChannel_Dial(t *testing.T) {
+	t.Run("handler should be called once, by dialing peer only", func(t *testing.T) {
+		report := test.CheckRoutines(t)
+		defer report()
+
+		dialCalls := make(chan bool, 2)
+		wg := new(sync.WaitGroup)
+		wg.Add(2)
+
+		offerPC, answerPC, err := newPair()
+		if err != nil {
+			t.Fatalf("Failed to create a PC pair for testing")
+		}
+
+		answerPC.OnDataChannel(func(d *DataChannel) {
+			if d.Label() != expectedLabel {
+				return
+			}
+
+			d.OnDial(func() {
+				// only dialing side should fire OnDial
+				t.Fatalf("answering side should not call on dial")
+			})
+
+			d.OnOpen(wg.Done)
+		})
+
+		d, err := offerPC.CreateDataChannel(expectedLabel, nil)
+		assert.NoError(t, err)
+		d.OnDial(func() {
+			dialCalls <- true
+			wg.Done()
+		})
+
+		assert.NoError(t, signalPair(offerPC, answerPC))
+
+		wg.Wait()
+		closePairNow(t, offerPC, answerPC)
+
+		assert.Len(t, dialCalls, 1)
+	})
+
+	t.Run("handler should be called immediately if already dialed", func(t *testing.T) {
+		report := test.CheckRoutines(t)
+		defer report()
+
+		done := make(chan bool)
+
+		offerPC, answerPC, err := newPair()
+		if err != nil {
+			t.Fatalf("Failed to create a PC pair for testing")
+		}
+
+		d, err := offerPC.CreateDataChannel(expectedLabel, nil)
+		assert.NoError(t, err)
+		d.OnOpen(func() {
+			// when the offer DC has been opened, its guaranteed to have dialed since it has
+			// received a response to said dial. this test represents an unrealistic usage,
+			// but its the best way to guarantee we "missed" the dial event and still invoke
+			// the handler.
+			d.OnDial(func() {
+				done <- true
+			})
+		})
+
+		assert.NoError(t, signalPair(offerPC, answerPC))
+
+		closePair(t, offerPC, answerPC, done)
+	})
+}