Repository: quic-go/quic-go
Pull request #4812 (2024-12-28 07:34:40)

don't generate empty STREAM frames in the send stream

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4812?dropdown=coverage&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:
> Project coverage is 83.78%. Comparing base [(`56bebc4`)](https://app.codecov.io/gh/quic-go/quic-go/commit/56bebc4e61c68535f5d11f22fefa1ff53024dabb?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`1160807`)](https://app.codecov.io/gh/quic-go/quic-go/commit/1160807833fad38982c171077180355f4c2bbf8e?dropdown=coverage&el=desc&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    #4812      +/-   ##
==========================================
- Coverage   83.86%   83.78%   -0.09%     
==========================================
  Files         149      146       -3     
  Lines       16052    15712     -340     
==========================================
- Hits        13462    13163     -299     
+ Misses       2059     2036      -23     
+ Partials      531      513      -18     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4812?dropdown=coverage&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/multiplexer.go b/multiplexer.go
index 85f7f403..cebe7c2e 100644
--- a/multiplexer.go
+++ b/multiplexer.go
@@ -67,7 +67,7 @@ func (m *connMultiplexer) RemoveConn(c indexableConn) error {
 
 	connIndex := m.index(c.LocalAddr())
 	if _, ok := m.conns[connIndex]; !ok {
-		return fmt.Errorf("cannote remove connection, connection is unknown")
+		return fmt.Errorf("cannote remove connection %s, connection is unknown", connIndex)
 	}
 
 	delete(m.conns, connIndex)
diff --git a/send_stream.go b/send_stream.go
index 36f04260..bbc5140e 100644
--- a/send_stream.go
+++ b/send_stream.go
@@ -277,6 +277,9 @@ func (s *sendStream) popNewOrRetransmittedStreamFrame(maxBytes protocol.ByteCoun
 	}
 
 	f, hasMoreData := s.popNewStreamFrame(maxBytes, sendWindow, v)
+	if f == nil {
+		return nil, hasMoreData, false
+	}
 	if dataLen := f.DataLen(); dataLen > 0 {
 		s.writeOffset += f.DataLen()
 		s.flowController.AddBytesSent(f.DataLen())
@@ -290,10 +293,12 @@ func (s *sendStream) popNewOrRetransmittedStreamFrame(maxBytes protocol.ByteCoun
 
 func (s *sendStream) popNewStreamFrame(maxBytes, sendWindow protocol.ByteCount, v protocol.Version) (*wire.StreamFrame, bool) {
 	if s.nextFrame != nil {
+		maxDataLen := min(sendWindow, s.nextFrame.MaxDataLen(maxBytes, v))
+		if maxDataLen == 0 {
+			return nil, true
+		}
 		nextFrame := s.nextFrame
 		s.nextFrame = nil
-
-		maxDataLen := min(sendWindow, nextFrame.MaxDataLen(maxBytes, v))
 		if nextFrame.DataLen() > maxDataLen {
 			s.nextFrame = wire.GetStreamFrame()
 			s.nextFrame.StreamID = s.streamID


Test patch:
diff --git a/integrationtests/self/mitm_test.go b/integrationtests/self/mitm_test.go
index a5b6acb0..11cc97cf 100644
--- a/integrationtests/self/mitm_test.go
+++ b/integrationtests/self/mitm_test.go
@@ -63,7 +63,7 @@ func TestMITMDuplicatePackets(t *testing.T) {
 	})
 }
 
-func TestMITInjectCorruptedPackets(t *testing.T) {
+func TestMITMInjectCorruptedPackets(t *testing.T) {
 	t.Run("towards the server", func(t *testing.T) {
 		testMITMInjectCorruptedPackets(t, quicproxy.DirectionIncoming)
 	})
@@ -142,7 +142,7 @@ func testMITMInjectRandomPackets(t *testing.T, direction quicproxy.Direction) {
 		return false
 	}
 
-	runMITMTest(t, serverTransport, clientTransport, direction, rtt, dropCallback)
+	runMITMTest(t, serverTransport, clientTransport, rtt, dropCallback)
 }
 
 func testMITMDuplicatePackets(t *testing.T, direction quicproxy.Direction) {
@@ -162,20 +162,19 @@ func testMITMDuplicatePackets(t *testing.T, direction quicproxy.Direction) {
 		return false
 	}
 
-	runMITMTest(t, serverTransport, clientTransport, direction, rtt, dropCallback)
+	runMITMTest(t, serverTransport, clientTransport, rtt, dropCallback)
 }
 
 func testMITMInjectCorruptedPackets(t *testing.T, direction quicproxy.Direction) {
 	serverTransport, clientTransport := getTransportsForMITMTest(t)
-	rtt := scaleDuration(10 * time.Millisecond)
+	rtt := scaleDuration(5 * time.Millisecond)
 
 	var numCorrupted atomic.Int32
-	const interval = 4
 	dropCallback := func(dir quicproxy.Direction, b []byte) bool {
 		if dir != direction {
 			return false
 		}
-		if rand.Intn(interval) == 0 {
+		if rand.Intn(4) == 0 {
 			numCorrupted.Add(1)
 			pos := rand.Intn(len(b))
 			b[pos] = byte(rand.Intn(256))
@@ -190,17 +189,12 @@ func testMITMInjectCorruptedPackets(t *testing.T, direction quicproxy.Direction)
 		return false
 	}
 
-	runMITMTest(t, serverTransport, clientTransport, direction, rtt, dropCallback)
+	runMITMTest(t, serverTransport, clientTransport, rtt, dropCallback)
 	require.NotZero(t, int(numCorrupted.Load()))
 }
 
-func runMITMTest(t *testing.T,
-	serverTransport, clientTransport *quic.Transport,
-	direction quicproxy.Direction,
-	rtt time.Duration,
-	dropCallback quicproxy.DropCallback,
-) {
-	ln, err := serverTransport.Listen(getTLSConfig(), getQuicConfig(nil))
+func runMITMTest(t *testing.T, serverTr, clientTr *quic.Transport, rtt time.Duration, dropCb quicproxy.DropCallback) {
+	ln, err := serverTr.Listen(getTLSConfig(), getQuicConfig(nil))
 	require.NoError(t, err)
 	defer ln.Close()
 
@@ -208,41 +202,64 @@ func runMITMTest(t *testing.T,
 	proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
 		RemoteAddr:  fmt.Sprintf("localhost:%d", serverPort),
 		DelayPacket: func(dir quicproxy.Direction, b []byte) time.Duration { return rtt / 2 },
-		DropPacket:  dropCallback,
+		DropPacket:  dropCb,
 	})
 	require.NoError(t, err)
 	defer proxy.Close()
 
-	conn, err := clientTransport.Dial(
-		context.Background(),
+	ctx, cancel := context.WithTimeout(context.Background(), time.Second)
+	defer cancel()
+	conn, err := clientTr.Dial(
+		ctx,
 		&net.UDPAddr{IP: net.IPv4(127, 0, 0, 1), Port: proxy.LocalPort()},
 		getTLSClientConfig(),
 		getQuicConfig(nil),
 	)
 	require.NoError(t, err)
+	defer conn.CloseWithError(0, "")
 
-	serverConn, err := ln.Accept(context.Background())
+	serverConn, err := ln.Accept(ctx)
 	require.NoError(t, err)
-	serverStr, err := serverConn.OpenUniStream()
+	defer serverConn.CloseWithError(0, "")
+
+	str, err := conn.OpenStreamSync(ctx)
 	require.NoError(t, err)
+	clientErrChan := make(chan error, 1)
+	go func() {
+		_, err := str.Write(PRData)
+		clientErrChan <- err
+		str.Close()
+	}()
 
-	errChan := make(chan error, 1)
+	serverStr, err := serverConn.AcceptStream(ctx)
+	require.NoError(t, err)
+	serverErrChan := make(chan error, 1)
 	go func() {
-		defer close(errChan)
-		if _, err := serverStr.Write(PRData); err != nil {
-			errChan <- err
+		defer close(serverErrChan)
+		if _, err := io.Copy(serverStr, serverStr); err != nil {
+			serverErrChan <- err
 			return
 		}
 		serverStr.Close()
 	}()
-	require.NoError(t, <-errChan)
+	require.NoError(t, <-serverErrChan)
 
-	str, err := conn.AcceptUniStream(context.Background())
-	require.NoError(t, err)
 	data, err := io.ReadAll(str)
 	require.NoError(t, err)
 	require.Equal(t, PRData, data)
-	require.NoError(t, conn.CloseWithError(0, ""))
+
+	select {
+	case err := <-clientErrChan:
+		require.NoError(t, err)
+	case <-time.After(time.Second):
+		t.Fatal("timeout")
+	}
+	select {
+	case err := <-serverErrChan:
+		require.NoError(t, err)
+	case <-time.After(time.Second):
+		t.Fatal("timeout")
+	}
 }
 
 func TestMITMForgedVersionNegotiationPacket(t *testing.T) {
diff --git a/multiplexer_test.go b/multiplexer_test.go
index 48590b28..b98dc92c 100644
--- a/multiplexer_test.go
+++ b/multiplexer_test.go
@@ -2,25 +2,32 @@ package quic
 
 import (
 	"net"
+	"testing"
 
-	. "github.com/onsi/ginkgo/v2"
-	. "github.com/onsi/gomega"
+	"github.com/stretchr/testify/require"
 )
 
-var _ = Describe("Multiplexer", func() {
-	It("adds new packet conns", func() {
-		conn1 := NewMockPacketConn(mockCtrl)
-		conn1.EXPECT().LocalAddr().Return(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1234})
-		getMultiplexer().AddConn(conn1)
-		conn2 := NewMockPacketConn(mockCtrl)
-		conn2.EXPECT().LocalAddr().Return(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1235})
-		getMultiplexer().AddConn(conn2)
-	})
-
-	It("panics when the same connection is added twice", func() {
-		conn := NewMockPacketConn(mockCtrl)
-		conn.EXPECT().LocalAddr().Return(&net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4321}).Times(2)
-		getMultiplexer().AddConn(conn)
-		Expect(func() { getMultiplexer().AddConn(conn) }).To(Panic())
-	})
-})
+type mockIndexableConn struct{ addr net.Addr }
+
+var _ indexableConn = &mockIndexableConn{}
+
+func (m *mockIndexableConn) LocalAddr() net.Addr { return m.addr }
+
+func TestMultiplexerAddNewPacketConns(t *testing.T) {
+	conn1 := &mockIndexableConn{addr: &net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1234}}
+	getMultiplexer().AddConn(conn1)
+	conn2 := &mockIndexableConn{addr: &net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 1235}}
+	getMultiplexer().AddConn(conn2)
+
+	require.NoError(t, getMultiplexer().RemoveConn(conn1))
+	require.NoError(t, getMultiplexer().RemoveConn(conn2))
+}
+
+func TestMultiplexerPanicsOnDuplicateConn(t *testing.T) {
+	conn := &mockIndexableConn{addr: &net.UDPAddr{IP: net.IPv4(1, 2, 3, 4), Port: 4321}}
+	getMultiplexer().AddConn(conn)
+	require.Panics(t, func() { getMultiplexer().AddConn(conn) })
+
+	require.NoError(t, getMultiplexer().RemoveConn(conn))
+	require.ErrorContains(t, getMultiplexer().RemoveConn(conn), "cannote remove connection")
+}
diff --git a/send_stream_test.go b/send_stream_test.go
index f4d3de04..7859820a 100644
--- a/send_stream_test.go
+++ b/send_stream_test.go
@@ -122,12 +122,11 @@ func TestSendStreamWriteData(t *testing.T) {
 	n, err = strWithTimeout.Write([]byte("foobaz"))
 	require.NoError(t, err)
 	require.Equal(t, 6, n)
-	mockFC.EXPECT().SendWindowSize().Return(protocol.MaxByteCount).Times(2)
+	mockFC.EXPECT().SendWindowSize().Return(protocol.MaxByteCount).Times(3)
 	mockFC.EXPECT().AddBytesSent(protocol.ByteCount(3)).Times(2)
-	// TODO(#4807): check that no empty frames are popped
-	// frame, ok, hasMore = str.popStreamFrame(expectedFrameHeaderLen(streamID, 10), protocol.Version1)
-	// require.False(t, ok)
-	// require.True(t, hasMore)
+	frame, ok, hasMore = str.popStreamFrame(expectedFrameHeaderLen(streamID, 10), protocol.Version1)
+	require.False(t, ok)
+	require.True(t, hasMore)
 	frame, ok, hasMore = str.popStreamFrame(expectedFrameHeaderLen(streamID, 10)+3, protocol.Version1)
 	require.True(t, ok)
 	require.True(t, hasMore)
@@ -156,8 +155,8 @@ func TestSendStreamLargeWrites(t *testing.T) {
 	rand.Read(data)
 	errChan := make(chan error, 1)
 	go func() {
-		defer str.Close()
 		_, err := (&writerWithTimeout{Writer: str, Timeout: time.Second}).Write(data)
+		str.Close()
 		errChan <- err
 	}()
 	select {