Repository: quic-go/quic-go
Pull request #4943 (2025-01-28 04:20:37)

fix busy-looping when pacing and the send queue blocks

codecov[bot]: ## [Codecov](https://app.codecov.io/gh/quic-go/quic-go/pull/4943?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) Report
Attention: Patch coverage is `50.00000%` with `1 line` in your changes missing coverage. Please review.
> Project coverage is 83.74%. Comparing base [(`6b9921b`)](https://app.codecov.io/gh/quic-go/quic-go/commit/6b9921bbfcb9bc95efb665ad37dd13d9fd134560?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go) to head [(`9276db5`)](https://app.codecov.io/gh/quic-go/quic-go/commit/9276db5a4fe1df5afc6d48b4273d4fedee38a2d4?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go).

| [Files with missing lines](https://app.codecov.io/gh/quic-go/quic-go/pull/4943?dropdown=coverage&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/4943?src=pr&el=tree&filepath=connection.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=quic-go#diff-Y29ubmVjdGlvbi5nbw==) | 50.00% | [1 Missing :warning: ](https://app.codecov.io/gh/quic-go/quic-go/pull/4943?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    #4943      +/-   ##
==========================================
- Coverage   83.85%   83.74%   -0.11%     
==========================================
  Files         153      150       -3     
  Lines       16492    16149     -343     
==========================================
- Hits        13829    13523     -306     
+ Misses       2136     2115      -21     
+ Partials      527      511      -16     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/quic-go/quic-go/pull/4943?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/connection.go b/connection.go
index df0ca314..b5287938 100644
--- a/connection.go
+++ b/connection.go
@@ -637,9 +637,10 @@ runLoop:
 		}
 
 		if s.sendQueue.WouldBlock() {
-			// The send queue is still busy sending out packets.
-			// Wait until there's space to enqueue new packets.
+			// The send queue is still busy sending out packets. Wait until there's space to enqueue new packets.
 			sendQueueAvailable = s.sendQueue.Available()
+			// Cancel the pacing timer, as we can't send any more packets until the send queue is available again.
+			s.pacingDeadline = time.Time{}
 			continue
 		}
 
@@ -652,7 +653,10 @@ runLoop:
 			break runLoop
 		}
 		if s.sendQueue.WouldBlock() {
+			// The send queue is still busy sending out packets. Wait until there's space to enqueue new packets.
 			sendQueueAvailable = s.sendQueue.Available()
+			// Cancel the pacing timer, as we can't send any more packets until the send queue is available again.
+			s.pacingDeadline = time.Time{}
 		} else {
 			sendQueueAvailable = nil
 		}


Test patch:
diff --git a/connection_test.go b/connection_test.go
index 57f49a03..58bb51ed 100644
--- a/connection_test.go
+++ b/connection_test.go
@@ -1724,6 +1724,61 @@ func TestConnectionPacketPacing(t *testing.T) {
 	}
 }
 
+// When the send queue blocks, we need to reset the pacing timer, otherwise the run loop might busy-loop.
+// See https://github.com/quic-go/quic-go/pull/4943 for more details.
+func TestConnectionPacingAndSendQueue(t *testing.T) {
+	mockCtrl := gomock.NewController(t)
+	sph := mockackhandler.NewMockSentPacketHandler(mockCtrl)
+	sender := NewMockSender(mockCtrl)
+
+	tc := newServerTestConnection(t,
+		mockCtrl,
+		nil,
+		false,
+		connectionOptSentPacketHandler(sph),
+		connectionOptSender(sender),
+		connectionOptHandshakeConfirmed(),
+		// set a fixed RTT, so that the idle timeout doesn't interfere with this test
+		connectionOptRTT(10*time.Second),
+	)
+	sender.EXPECT().Run()
+
+	sendQueueAvailable := make(chan struct{})
+	pacingDeadline := time.Now().Add(-time.Millisecond)
+	var counter int
+	// allow exactly one packet to be sent, then become blocked
+	sender.EXPECT().WouldBlock().Return(false)
+	sender.EXPECT().WouldBlock().DoAndReturn(func() bool { counter++; return true }).AnyTimes()
+	sender.EXPECT().Available().Return(sendQueueAvailable).AnyTimes()
+	sph.EXPECT().GetLossDetectionTimeout().Return(time.Now().Add(time.Hour)).AnyTimes()
+	sph.EXPECT().SendMode(gomock.Any()).Return(ackhandler.SendPacingLimited).AnyTimes()
+	sph.EXPECT().TimeUntilSend().Return(pacingDeadline).AnyTimes()
+	sph.EXPECT().ECNMode(gomock.Any()).Return(protocol.ECNNon).AnyTimes()
+	tc.packer.EXPECT().PackAckOnlyPacket(gomock.Any(), gomock.Any(), gomock.Any()).Return(
+		shortHeaderPacket{}, nil, errNothingToPack,
+	)
+
+	errChan := make(chan error, 1)
+	go func() { errChan <- tc.conn.run() }()
+	tc.conn.scheduleSending()
+
+	time.Sleep(scaleDuration(10 * time.Millisecond))
+
+	// test teardown
+	tc.connRunner.EXPECT().Remove(gomock.Any()).AnyTimes()
+	sender.EXPECT().Close()
+	tc.conn.destroy(nil)
+	select {
+	case err := <-errChan:
+		require.NoError(t, err)
+	case <-time.After(time.Second):
+		t.Fatal("timeout")
+	}
+
+	// make sure the run loop didn't do too many iterations
+	require.Less(t, counter, 3)
+}
+
 func TestConnectionIdleTimeout(t *testing.T) {
 	mockCtrl := gomock.NewController(t)
 	sph := mockackhandler.NewMockSentPacketHandler(mockCtrl)