Repository: quic-go/quic-go
Pull request #4306 (2024-02-03 05:46:48)

fix flaky 0-RTT packet drop test
We don't really need to _randomly_ drop packets. It should be fine to predictably drop packets instead. That way, we can't run into the (unlikely) corner case where the random number generator decides to not drop any packets at all.

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4306?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Comparison is base [(`07ec324`)](https://app.codecov.io/gh/quic-go/quic-go/commit/07ec3245bd4ac4c510b6905b5f1c443f7273c08a?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) 84.14% compared to head [(`cc68af9`)](https://app.codecov.io/gh/quic-go/quic-go/pull/4306?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) 84.16%.


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


```diff
@@            Coverage Diff             @@
##           master    #4306      +/-   ##
==========================================
+ Coverage   84.14%   84.16%   +0.02%     
==========================================
  Files         150      150              
  Lines       15364    15364              
==========================================
+ Hits        12928    12931       +3     
+ Misses       1938     1935       -3     
  Partials      498      498              
```



</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4306?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).


Patch:
diff --git a/internal/flowcontrol/base_flow_controller.go b/internal/flowcontrol/base_flow_controller.go
index 184aad34..3d88d577 100644
--- a/internal/flowcontrol/base_flow_controller.go
+++ b/internal/flowcontrol/base_flow_controller.go
@@ -48,10 +48,12 @@ func (c *baseFlowController) AddBytesSent(n protocol.ByteCount) {
 }
 
 // UpdateSendWindow is called after receiving a MAX_{STREAM_}DATA frame.
-func (c *baseFlowController) UpdateSendWindow(offset protocol.ByteCount) {
+func (c *baseFlowController) UpdateSendWindow(offset protocol.ByteCount) (updated bool) {
 	if offset > c.sendWindow {
 		c.sendWindow = offset
+		return true
 	}
+	return false
 }
 
 func (c *baseFlowController) sendWindowSize() protocol.ByteCount {
diff --git a/internal/flowcontrol/interface.go b/internal/flowcontrol/interface.go
index 946519d5..fc5f9de0 100644
--- a/internal/flowcontrol/interface.go
+++ b/internal/flowcontrol/interface.go
@@ -5,7 +5,7 @@ import "github.com/quic-go/quic-go/internal/protocol"
 type flowController interface {
 	// for sending
 	SendWindowSize() protocol.ByteCount
-	UpdateSendWindow(protocol.ByteCount)
+	UpdateSendWindow(protocol.ByteCount) (updated bool)
 	AddBytesSent(protocol.ByteCount)
 	// for receiving
 	AddBytesRead(protocol.ByteCount)
@@ -16,12 +16,11 @@ type flowController interface {
 // A StreamFlowController is a flow controller for a QUIC stream.
 type StreamFlowController interface {
 	flowController
-	// for receiving
-	// UpdateHighestReceived should be called when a new highest offset is received
+	// UpdateHighestReceived is called when a new highest offset is received
 	// final has to be to true if this is the final offset of the stream,
 	// as contained in a STREAM frame with FIN bit, and the RESET_STREAM frame
 	UpdateHighestReceived(offset protocol.ByteCount, final bool) error
-	// Abandon should be called when reading from the stream is aborted early,
+	// Abandon is called when reading from the stream is aborted early,
 	// and there won't be any further calls to AddBytesRead.
 	Abandon()
 }
diff --git a/internal/mocks/connection_flow_controller.go b/internal/mocks/connection_flow_controller.go
index ac328602..5f6cb2aa 100644
--- a/internal/mocks/connection_flow_controller.go
+++ b/internal/mocks/connection_flow_controller.go
@@ -264,9 +264,11 @@ func (c *ConnectionFlowControllerSendWindowSizeCall) DoAndReturn(f func() protoc
 }
 
 // UpdateSendWindow mocks base method.
-func (m *MockConnectionFlowController) UpdateSendWindow(arg0 protocol.ByteCount) {
+func (m *MockConnectionFlowController) UpdateSendWindow(arg0 protocol.ByteCount) bool {
 	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "UpdateSendWindow", arg0)
+	ret := m.ctrl.Call(m, "UpdateSendWindow", arg0)
+	ret0, _ := ret[0].(bool)
+	return ret0
 }
 
 // UpdateSendWindow indicates an expected call of UpdateSendWindow.
@@ -282,19 +284,19 @@ type ConnectionFlowControllerUpdateSendWindowCall struct {
 }
 
 // Return rewrite *gomock.Call.Return
-func (c *ConnectionFlowControllerUpdateSendWindowCall) Return() *ConnectionFlowControllerUpdateSendWindowCall {
-	c.Call = c.Call.Return()
+func (c *ConnectionFlowControllerUpdateSendWindowCall) Return(arg0 bool) *ConnectionFlowControllerUpdateSendWindowCall {
+	c.Call = c.Call.Return(arg0)
 	return c
 }
 
 // Do rewrite *gomock.Call.Do
-func (c *ConnectionFlowControllerUpdateSendWindowCall) Do(f func(protocol.ByteCount)) *ConnectionFlowControllerUpdateSendWindowCall {
+func (c *ConnectionFlowControllerUpdateSendWindowCall) Do(f func(protocol.ByteCount) bool) *ConnectionFlowControllerUpdateSendWindowCall {
 	c.Call = c.Call.Do(f)
 	return c
 }
 
 // DoAndReturn rewrite *gomock.Call.DoAndReturn
-func (c *ConnectionFlowControllerUpdateSendWindowCall) DoAndReturn(f func(protocol.ByteCount)) *ConnectionFlowControllerUpdateSendWindowCall {
+func (c *ConnectionFlowControllerUpdateSendWindowCall) DoAndReturn(f func(protocol.ByteCount) bool) *ConnectionFlowControllerUpdateSendWindowCall {
 	c.Call = c.Call.DoAndReturn(f)
 	return c
 }
diff --git a/internal/mocks/stream_flow_controller.go b/internal/mocks/stream_flow_controller.go
index 7207d760..2e244dfb 100644
--- a/internal/mocks/stream_flow_controller.go
+++ b/internal/mocks/stream_flow_controller.go
@@ -300,9 +300,11 @@ func (c *StreamFlowControllerUpdateHighestReceivedCall) DoAndReturn(f func(proto
 }
 
 // UpdateSendWindow mocks base method.
-func (m *MockStreamFlowController) UpdateSendWindow(arg0 protocol.ByteCount) {
+func (m *MockStreamFlowController) UpdateSendWindow(arg0 protocol.ByteCount) bool {
 	m.ctrl.T.Helper()
-	m.ctrl.Call(m, "UpdateSendWindow", arg0)
+	ret := m.ctrl.Call(m, "UpdateSendWindow", arg0)
+	ret0, _ := ret[0].(bool)
+	return ret0
 }
 
 // UpdateSendWindow indicates an expected call of UpdateSendWindow.
@@ -318,19 +320,19 @@ type StreamFlowControllerUpdateSendWindowCall struct {
 }
 
 // Return rewrite *gomock.Call.Return
-func (c *StreamFlowControllerUpdateSendWindowCall) Return() *StreamFlowControllerUpdateSendWindowCall {
-	c.Call = c.Call.Return()
+func (c *StreamFlowControllerUpdateSendWindowCall) Return(arg0 bool) *StreamFlowControllerUpdateSendWindowCall {
+	c.Call = c.Call.Return(arg0)
 	return c
 }
 
 // Do rewrite *gomock.Call.Do
-func (c *StreamFlowControllerUpdateSendWindowCall) Do(f func(protocol.ByteCount)) *StreamFlowControllerUpdateSendWindowCall {
+func (c *StreamFlowControllerUpdateSendWindowCall) Do(f func(protocol.ByteCount) bool) *StreamFlowControllerUpdateSendWindowCall {
 	c.Call = c.Call.Do(f)
 	return c
 }
 
 // DoAndReturn rewrite *gomock.Call.DoAndReturn
-func (c *StreamFlowControllerUpdateSendWindowCall) DoAndReturn(f func(protocol.ByteCount)) *StreamFlowControllerUpdateSendWindowCall {
+func (c *StreamFlowControllerUpdateSendWindowCall) DoAndReturn(f func(protocol.ByteCount) bool) *StreamFlowControllerUpdateSendWindowCall {
 	c.Call = c.Call.DoAndReturn(f)
 	return c
 }
diff --git a/send_stream.go b/send_stream.go
index f4ae5b2a..e1ce3e67 100644
--- a/send_stream.go
+++ b/send_stream.go
@@ -404,11 +404,13 @@ func (s *sendStream) cancelWriteImpl(errorCode qerr.StreamErrorCode, remote bool
 }
 
 func (s *sendStream) updateSendWindow(limit protocol.ByteCount) {
+	updated := s.flowController.UpdateSendWindow(limit)
+	if !updated { // duplicate or reordered MAX_STREAM_DATA frame
+		return
+	}
 	s.mutex.Lock()
 	hasStreamData := s.dataForWriting != nil || s.nextFrame != nil
 	s.mutex.Unlock()
-
-	s.flowController.UpdateSendWindow(limit)
 	if hasStreamData {
 		s.sender.onHasStreamData(s.streamID)
 	}


Test patch:
diff --git a/integrationtests/self/zero_rtt_test.go b/integrationtests/self/zero_rtt_test.go
index 85ad015e..55307679 100644
--- a/integrationtests/self/zero_rtt_test.go
+++ b/integrationtests/self/zero_rtt_test.go
@@ -5,7 +5,6 @@ import (
 	"crypto/tls"
 	"fmt"
 	"io"
-	mrand "math/rand"
 	"net"
 	"sync"
 	"sync/atomic"
@@ -380,7 +379,7 @@ var _ = Describe("0-RTT", func() {
 	})
 
 	It("transfers 0-RTT data, when 0-RTT packets are lost", func() {
-		var num0RTTPackets, num0RTTDropped atomic.Uint32
+		var num0RTTPackets, numDropped atomic.Uint32
 
 		tlsConf := getTLSConfig()
 		clientConf := getTLSClientConfig()
@@ -399,17 +398,8 @@ var _ = Describe("0-RTT", func() {
 		defer ln.Close()
 
 		proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
-			RemoteAddr: fmt.Sprintf("localhost:%d", ln.Addr().(*net.UDPAddr).Port),
-			DelayPacket: func(_ quicproxy.Direction, data []byte) time.Duration {
-				if wire.IsLongHeaderPacket(data[0]) {
-					hdr, _, _, err := wire.ParsePacket(data)
-					Expect(err).ToNot(HaveOccurred())
-					if hdr.Type == protocol.PacketType0RTT {
-						num0RTTPackets.Add(1)
-					}
-				}
-				return rtt / 2
-			},
+			RemoteAddr:  fmt.Sprintf("localhost:%d", ln.Addr().(*net.UDPAddr).Port),
+			DelayPacket: func(_ quicproxy.Direction, data []byte) time.Duration { return rtt / 2 },
 			DropPacket: func(_ quicproxy.Direction, data []byte) bool {
 				if !wire.IsLongHeaderPacket(data[0]) {
 					return false
@@ -417,10 +407,11 @@ var _ = Describe("0-RTT", func() {
 				hdr, _, _, err := wire.ParsePacket(data)
 				Expect(err).ToNot(HaveOccurred())
 				if hdr.Type == protocol.PacketType0RTT {
+					count := num0RTTPackets.Add(1)
 					// drop 25% of the 0-RTT packets
-					drop := mrand.Intn(4) == 0
+					drop := count%4 == 0
 					if drop {
-						num0RTTDropped.Add(1)
+						numDropped.Add(1)
 					}
 					return drop
 				}
@@ -433,9 +424,8 @@ var _ = Describe("0-RTT", func() {
 		transfer0RTTData(ln, proxy.LocalPort(), protocol.DefaultConnectionIDLength, clientConf, nil, PRData)
 
 		num0RTT := num0RTTPackets.Load()
-		numDropped := num0RTTDropped.Load()
-		fmt.Fprintf(GinkgoWriter, "Sent %d 0-RTT packets. Dropped %d of those.", num0RTT, numDropped)
-		Expect(numDropped).ToNot(BeZero())
+		fmt.Fprintf(GinkgoWriter, "Sent %d 0-RTT packets. Dropped %d of those.", num0RTT, numDropped.Load())
+		Expect(numDropped.Load()).ToNot(BeZero())
 		Expect(num0RTT).ToNot(BeZero())
 		Expect(get0RTTPackets(counter.getRcvdLongHeaderPackets())).ToNot(BeEmpty())
 	})
diff --git a/internal/flowcontrol/base_flow_controller_test.go b/internal/flowcontrol/base_flow_controller_test.go
index 6c26e7f8..da6cf357 100644
--- a/internal/flowcontrol/base_flow_controller_test.go
+++ b/internal/flowcontrol/base_flow_controller_test.go
@@ -59,9 +59,9 @@ var _ = Describe("Base Flow controller", func() {
 		})
 
 		It("does not decrease the flow control window", func() {
-			controller.UpdateSendWindow(20)
+			Expect(controller.UpdateSendWindow(20)).To(BeTrue())
 			Expect(controller.sendWindowSize()).To(Equal(protocol.ByteCount(20)))
-			controller.UpdateSendWindow(10)
+			Expect(controller.UpdateSendWindow(10)).To(BeFalse())
 			Expect(controller.sendWindowSize()).To(Equal(protocol.ByteCount(20)))
 		})
 
diff --git a/send_stream_test.go b/send_stream_test.go
index ad1d0469..50713308 100644
--- a/send_stream_test.go
+++ b/send_stream_test.go
@@ -682,7 +682,7 @@ var _ = Describe("Send Stream", func() {
 		})
 
 		It("says when it has data for sending", func() {
-			mockFC.EXPECT().UpdateSendWindow(gomock.Any())
+			mockFC.EXPECT().UpdateSendWindow(gomock.Any()).Return(true)
 			mockSender.EXPECT().onHasStreamData(streamID)
 			done := make(chan struct{})
 			go func() {
@@ -698,6 +698,24 @@ var _ = Describe("Send Stream", func() {
 			str.closeForShutdown(nil)
 			Eventually(done).Should(BeClosed())
 		})
+
+		It("doesn't say it has data for sending if the MAX_STREAM_DATA frame was reordered", func() {
+			mockFC.EXPECT().UpdateSendWindow(gomock.Any()).Return(false) // reordered frame
+			mockSender.EXPECT().onHasStreamData(streamID)
+			done := make(chan struct{})
+			go func() {
+				defer GinkgoRecover()
+				_, err := str.Write([]byte("foobar"))
+				Expect(err).ToNot(HaveOccurred())
+				close(done)
+			}()
+			waitForWrite()
+			// don't expect any calls to onHasStreamData
+			str.updateSendWindow(42)
+			// make sure the Write go routine returns
+			str.closeForShutdown(nil)
+			Eventually(done).Should(BeClosed())
+		})
 	})
 
 	Context("stream cancellations", func() {