Repository: quic-go/quic-go
Pull request #4143 (2023-10-30 13:46:16)

send large max_datagram_frame size, introduce a DatagramTooLargeError error
The size can be overwritten to a lower value for testing. This implements the suggestion from https://github.com/quic-go/quic-go/issues/3300#issuecomment-1642475753.

google-cla[bot]: Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this [failed invocation](https://github.com/quic-go/quic-go/pull/4143/checks?check_run_id=18185436458) of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.
chungthuang: Hi @marten-seemann, thanks for reviewing. Do you have other feedback for this PR?
marten-seemann: > The size can be overwritten to a lower value for testing.

Do you want to add a test case?
chungthuang: > > The size can be overwritten to a lower value for testing.
> 
> Do you want to add a test case?
https://github.com/quic-go/quic-go/pull/4143/commits/257b644b4237bcd2be1957139065b0cce9170de6 adds a test case to make sure peer's max DATAGRAM size is respected.
I think the other case of making sure only DATAGRAM frame that can fit into a packet is sent is already covered in https://github.com/quic-go/quic-go/blob/master/packet_packer_test.go#L594.
chungthuang: Hi @marten-seemann, I can squash the commits if you don't have other feedback.
chungthuang: Hi @marten-seemann, just want to check if this is good to merge soon?
marten-seemann: Sorry for the delay @chungthuang! Tests are failing, can you please take a look?
chungthuang: > Sorry for the delay @chungthuang! Tests are failing, can you please take a look?

No worries. The tests were failing because `internal/wire/transport_parameter_test.go` was still using `protocol.MaxDatagramFrameSize`. This is fixed now.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
Attention: `10 lines` in your changes are missing coverage. Please review.
> Comparison is base [(`a360354`)](https://app.codecov.io/gh/quic-go/quic-go/commit/a3603549eebd4e22fd8ea4d4fe0c9011bc08daa4?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) 83.75% compared to head [(`6de8be9`)](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) 83.72%.
> Report is 9 commits behind head on master.

| [Files](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) | Patch % | Lines |
|---|---|---|
| [connection.go](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go#diff-Y29ubmVjdGlvbi5nbw==) | 0.00% | [6 Missing :warning: ](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) |
| [errors.go](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go#diff-ZXJyb3JzLmdv) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) |

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


```diff
@@            Coverage Diff             @@
##           master    #4143      +/-   ##
==========================================
- Coverage   83.75%   83.72%   -0.02%     
==========================================
  Files         150      150              
  Lines       15411    15434      +23     
==========================================
+ Hits        12906    12922      +16     
- Misses       2007     2014       +7     
  Partials      498      498              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4143?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

chungthuang: Looks like all tests passed, but I don't have permission to merge.

Patch:
diff --git a/connection.go b/connection.go
index 7f26ef5e..1b288a2b 100644
--- a/connection.go
+++ b/connection.go
@@ -307,7 +307,7 @@ var newConnection = func(
 		RetrySourceConnectionID:   retrySrcConnID,
 	}
 	if s.config.EnableDatagrams {
-		params.MaxDatagramFrameSize = protocol.MaxDatagramFrameSize
+		params.MaxDatagramFrameSize = wire.MaxDatagramSize
 	} else {
 		params.MaxDatagramFrameSize = protocol.InvalidByteCount
 	}
@@ -414,7 +414,7 @@ var newClientConnection = func(
 		InitialSourceConnectionID: srcConnID,
 	}
 	if s.config.EnableDatagrams {
-		params.MaxDatagramFrameSize = protocol.MaxDatagramFrameSize
+		params.MaxDatagramFrameSize = wire.MaxDatagramSize
 	} else {
 		params.MaxDatagramFrameSize = protocol.InvalidByteCount
 	}
@@ -1522,7 +1522,7 @@ func (s *connection) handleAckFrame(frame *wire.AckFrame, encLevel protocol.Encr
 }
 
 func (s *connection) handleDatagramFrame(f *wire.DatagramFrame) error {
-	if f.Length(s.version) > protocol.MaxDatagramFrameSize {
+	if f.Length(s.version) > wire.MaxDatagramSize {
 		return &qerr.TransportError{
 			ErrorCode:    qerr.ProtocolViolation,
 			ErrorMessage: "DATAGRAM frame too large",
@@ -2350,7 +2350,9 @@ func (s *connection) SendDatagram(p []byte) error {
 
 	f := &wire.DatagramFrame{DataLenPresent: true}
 	if protocol.ByteCount(len(p)) > f.MaxDataLen(s.peerParams.MaxDatagramFrameSize, s.version) {
-		return errors.New("message too large")
+		return &DatagramTooLargeError{
+			PeerMaxDatagramFrameSize: int64(s.peerParams.MaxDatagramFrameSize),
+		}
 	}
 	f.Data = make([]byte, len(p))
 	copy(f.Data, p)
diff --git a/errors.go b/errors.go
index c9fb0a07..fda3c924 100644
--- a/errors.go
+++ b/errors.go
@@ -61,3 +61,15 @@ func (e *StreamError) Error() string {
 	}
 	return fmt.Sprintf("stream %d canceled by %s with error code %d", e.StreamID, pers, e.ErrorCode)
 }
+
+// DatagramTooLargeError is returned from Connection.SendDatagram if the payload is too large to be sent.
+type DatagramTooLargeError struct {
+	PeerMaxDatagramFrameSize int64
+}
+
+func (e *DatagramTooLargeError) Is(target error) bool {
+	_, ok := target.(*DatagramTooLargeError)
+	return ok
+}
+
+func (e *DatagramTooLargeError) Error() string { return "DATAGRAM frame too large" }
diff --git a/internal/protocol/params.go b/internal/protocol/params.go
index 3ca68bf8..28b6da7c 100644
--- a/internal/protocol/params.go
+++ b/internal/protocol/params.go
@@ -129,10 +129,6 @@ const MaxPostHandshakeCryptoFrameSize = 1000
 // but must ensure that a maximum size ACK frame fits into one packet.
 const MaxAckFrameSize ByteCount = 1000
 
-// MaxDatagramFrameSize is the maximum size of a DATAGRAM frame (RFC 9221).
-// The size is chosen such that a DATAGRAM frame fits into a QUIC packet.
-const MaxDatagramFrameSize ByteCount = 1200
-
 // DatagramRcvQueueLen is the length of the receive queue for DATAGRAM frames (RFC 9221)
 const DatagramRcvQueueLen = 128
 
diff --git a/internal/wire/datagram_frame.go b/internal/wire/datagram_frame.go
index e6c45196..4d001084 100644
--- a/internal/wire/datagram_frame.go
+++ b/internal/wire/datagram_frame.go
@@ -8,6 +8,12 @@ import (
 	"github.com/quic-go/quic-go/quicvarint"
 )
 
+// MaxDatagramSize is the maximum size of a DATAGRAM frame (RFC 9221).
+// By setting it to a large value, we allow all datagrams that fit into a QUIC packet.
+// The value is chosen such that it can still be encoded as a 2 byte varint.
+// This is a var and not a const so it can be set in tests.
+var MaxDatagramSize protocol.ByteCount = 16383
+
 // A DatagramFrame is a DATAGRAM frame
 type DatagramFrame struct {
 	DataLenPresent bool


Test patch:
diff --git a/integrationtests/self/datagram_test.go b/integrationtests/self/datagram_test.go
index 55e85771..f9608272 100644
--- a/integrationtests/self/datagram_test.go
+++ b/integrationtests/self/datagram_test.go
@@ -19,7 +19,8 @@ import (
 )
 
 var _ = Describe("Datagram test", func() {
-	const num = 100
+	const concurrentSends = 100
+	const maxDatagramSize = 250
 
 	var (
 		serverConn, clientConn *net.UDPConn
@@ -47,11 +48,11 @@ var _ = Describe("Datagram test", func() {
 
 			if expectDatagramSupport {
 				Expect(conn.ConnectionState().SupportsDatagrams).To(BeTrue())
-
 				if enableDatagram {
+					f := &wire.DatagramFrame{DataLenPresent: true}
 					var wg sync.WaitGroup
-					wg.Add(num)
-					for i := 0; i < num; i++ {
+					wg.Add(concurrentSends)
+					for i := 0; i < concurrentSends; i++ {
 						go func(i int) {
 							defer GinkgoRecover()
 							defer wg.Done()
@@ -60,6 +61,11 @@ var _ = Describe("Datagram test", func() {
 							Expect(conn.SendDatagram(b)).To(Succeed())
 						}(i)
 					}
+					maxDatagramMessageSize := f.MaxDataLen(maxDatagramSize, conn.ConnectionState().Version)
+					b := make([]byte, maxDatagramMessageSize+1)
+					Expect(conn.SendDatagram(b)).To(MatchError(&quic.DatagramTooLargeError{
+						PeerMaxDatagramFrameSize: int64(maxDatagramMessageSize),
+					}))
 					wg.Wait()
 				}
 			} else {
@@ -103,6 +109,8 @@ var _ = Describe("Datagram test", func() {
 	})
 
 	It("sends datagrams", func() {
+		oldMaxDatagramSize := wire.MaxDatagramSize
+		wire.MaxDatagramSize = maxDatagramSize
 		proxyPort, close := startServerAndProxy(true, true)
 		defer close()
 		raddr, err := net.ResolveUDPAddr("udp", fmt.Sprintf("localhost:%d", proxyPort))
@@ -128,14 +136,15 @@ var _ = Describe("Datagram test", func() {
 		}
 
 		numDropped := int(dropped.Load())
-		expVal := num - numDropped
+		expVal := concurrentSends - numDropped
 		fmt.Fprintf(GinkgoWriter, "Dropped %d out of %d packets.\n", numDropped, total.Load())
-		fmt.Fprintf(GinkgoWriter, "Received %d out of %d sent datagrams.\n", counter, num)
+		fmt.Fprintf(GinkgoWriter, "Received %d out of %d sent datagrams.\n", counter, concurrentSends)
 		Expect(counter).To(And(
 			BeNumerically(">", expVal*9/10),
-			BeNumerically("<", num),
+			BeNumerically("<", concurrentSends),
 		))
 		Eventually(conn.Context().Done).Should(BeClosed())
+		wire.MaxDatagramSize = oldMaxDatagramSize
 	})
 
 	It("server can disable datagram", func() {
diff --git a/internal/wire/transport_parameter_test.go b/internal/wire/transport_parameter_test.go
index 2fb79539..a82b6dcf 100644
--- a/internal/wire/transport_parameter_test.go
+++ b/internal/wire/transport_parameter_test.go
@@ -503,7 +503,7 @@ var _ = Describe("Transport Parameters", func() {
 				MaxBidiStreamNum:               protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
 				MaxUniStreamNum:                protocol.StreamNum(getRandomValueUpTo(int64(protocol.MaxStreamCount))),
 				ActiveConnectionIDLimit:        2 + getRandomValueUpTo(math.MaxInt64-2),
-				MaxDatagramFrameSize:           protocol.ByteCount(getRandomValueUpTo(int64(protocol.MaxDatagramFrameSize))),
+				MaxDatagramFrameSize:           protocol.ByteCount(getRandomValueUpTo(int64(MaxDatagramSize))),
 			}
 			Expect(params.ValidFor0RTT(params)).To(BeTrue())
 			b := params.MarshalForSessionTicket(nil)