From d31efd04c4482192099da6e8a04afa3c5e5d0690 Mon Sep 17 00:00:00 2001
From: boks1971 <raja.gobi@tutanota.com>
Date: Fri, 16 Aug 2024 16:30:26 +0530
Subject: [PATCH] Commit reliability on client after receiving ACK

From RFC 8832
https://www.rfc-editor.org/rfc/rfc8832.html#name-procedures, messages
should be ordered till dialing/opening side receives ACK.

```
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. After the DATA_CHANNEL_ACK or any
other message has been received on the data channel,
messages containing user data MUST be sent ordered on
ordered data channels and MUST be sent unordered on
unordered data channels. Therefore, receiving a message
containing user data on an unused stream indicates an error.
In that case, the corresponding data channel MUST be closed,
as described in [RFC8831].
```

Without waiting, the data channel open failed on Accept side as the
first message did not have PPI of DCEP. It got a user message
and that caused the Accept to fail.
---
 datachannel.go      | 10 ++++------
 datachannel_test.go | 26 ++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/datachannel.go b/datachannel.go
index 0050185..606444b 100644
--- a/datachannel.go
+++ b/datachannel.go
@@ -118,12 +118,7 @@ func Client(stream *sctp.Stream, config *Config) (*DataChannel, error) {
 			return nil, fmt.Errorf("failed to send ChannelOpen %w", err)
 		}
 	}
-	dc := newDataChannel(stream, config)
-
-	if err := dc.commitReliabilityParams(); err != nil {
-		return nil, err
-	}
-	return dc, nil
+	return newDataChannel(stream, config), nil
 }
 
 // Accept is used to accept incoming data channels over SCTP
@@ -285,6 +280,9 @@ func (c *DataChannel) handleDCEP(data []byte) error {
 
 	switch msg := msg.(type) {
 	case *channelAck:
+		if err := c.commitReliabilityParams(); err != nil {
+			return err
+		}
 		c.onOpenComplete()
 	default:
 		return fmt.Errorf("%w, wanted ACK got %v", ErrUnexpectedDataChannelType, msg)
diff --git a/datachannel_test.go b/datachannel_test.go
index 9c4965f..0df30d6 100644
--- a/datachannel_test.go
+++ b/datachannel_test.go
@@ -384,8 +384,34 @@ func TestDataChannel(t *testing.T) {
 		assert.True(t, reflect.DeepEqual(dc0.Config, *cfg), "local config should match")
 		assert.True(t, reflect.DeepEqual(dc1.Config, *cfg), "remote config should match")
 
+		// reliability parameters are committed after data channel open ACK is received on client side,
+		// wait for open to be completed
+		openCompleted := make(chan bool)
+		dc0.OnOpen(func() {
+			close(openCompleted)
+		})
+
 		var n int
 
+		// write a message as ReadChannel loops till user data is read
+		bridgeProcessAtLeastOne(br)
+		binary.BigEndian.PutUint32(sbuf, 10)
+		n, err = dc1.WriteDataChannel(sbuf, true)
+		assert.Nil(t, err, "Write() should succeed")
+		assert.Equal(t, len(sbuf), n, "data length should match")
+
+		// read data channel open ACK and the user message sent above
+		bridgeProcessAtLeastOne(br)
+		_, _, err = dc0.ReadDataChannel(rbuf)
+		assert.NoError(t, err)
+
+		select {
+		case <-time.After(time.Second * 10):
+			assert.FailNow(t, "OnOpen() failed to fire 10s")
+		case <-openCompleted:
+		}
+
+		// test unordered messages
 		binary.BigEndian.PutUint32(sbuf, 1)
 		n, err = dc0.WriteDataChannel(sbuf, true)
 		assert.Nil(t, err, "Write() should succeed")