commit 1b91de7bc22f286c2231de9489f625ee0dd15379
author dmcardle <dmcardle@google.com> Wed Jun 18 10:47:04 2025 -0700
committer Copybara-Service <copybara-worker@google.com> Wed Jun 18 10:48:08 2025 -0700
tree d08d37aa98e5a7df29f645cca555384a814ee8f0
parent 53c4506638a09b811eb9ceb15d7e45e15a5f423d

Fix data races in end-to-end tests

I discovered that I can reliably reproduce data races in two end-to-end tests by repeating the tests 100 times with TSan enabled. Like so:

```
blaze test --config=tsan --runs_per_test=100 //third_party/quic/core/http:end_to_end_test
```

The fix I came up with is to pause the server before calling `GetServerConnection()` and `GetServerSession()`. Both of these functions call, or transitively call, `GetDispatcher()`, which requires that `server_thread_` is paused.

Perhaps we can leverage thread safety annotations to make the compiler check this in the future.

PiperOrigin-RevId: 772980453

quiche/quic/core/http/end_to_end_test.cc

diff --git a/quiche/quic/core/http/end_to_end_test.cc b/quiche/quic/core/http/end_to_end_test.cc
index 6ae847b..090ef6d 100644
--- a/quiche/quic/core/http/end_to_end_test.cc
+++ b/quiche/quic/core/http/end_to_end_test.cc
@@ -444,6 +444,7 @@
     return client_session->connection();
   }
 
+  // Must be called while `server_thread_` is paused.
   QuicConnection* GetServerConnection() {
     QuicSpdySession* server_session = GetServerSession();
     if (server_session == nullptr) {
@@ -453,6 +454,7 @@
     return server_session->connection();
   }
 
+  // Must be called while `server_thread_` is paused.
   QuicSpdySession* GetServerSession() {
     QuicDispatcher* dispatcher = GetDispatcher();
     if (dispatcher == nullptr) {
@@ -468,7 +470,7 @@
         QuicDispatcherPeer::GetFirstSessionIfAny(dispatcher));
   }
 
-  // Must be called while server_thread_ is paused.
+  // Must be called while `server_thread_` is paused.
   QuicDispatcher* GetDispatcher() {
     if (!server_thread_) {
       ADD_FAILURE() << "Missing server thread";
@@ -482,7 +484,7 @@
     return QuicServerPeer::GetDispatcher(quic_server);
   }
 
-  // Must be called while server_thread_ is paused.
+  // Must be called while `server_thread_` is paused.
   const QuicDispatcherStats& GetDispatcherStats() {
     return GetDispatcher()->stats();
   }
@@ -7287,7 +7289,9 @@
 
   SendSynchronousFooRequestAndCheckResponse();
 
+  server_thread_->Pause();
   QuicSpdySession* server_session = GetServerSession();
+  server_thread_->Resume();
   EXPECT_FALSE(GetClientSession()->ShouldBufferRequestsUntilSettings());
   server_thread_->ScheduleAndWaitForCompletion([server_session] {
     EXPECT_TRUE(server_session->ShouldBufferRequestsUntilSettings());
@@ -8191,25 +8195,27 @@
   // Wait for handshake to complete, so that we can manipulate the server
   // connection without race conditions.
   server_thread_->WaitForCryptoHandshakeConfirmed();
+  server_thread_->Pause();
   QuicConnection* server_connection = GetServerConnection();
   QuicConnectionPeer::DisableEcnCodepointValidation(server_connection);
   QuicEcnCounts* ecn = QuicSentPacketManagerPeer::GetPeerEcnCounts(
       QuicConnectionPeer::GetSentPacketManager(server_connection),
       APPLICATION_DATA);
   EXPECT_TRUE(server_connection->set_ecn_codepoint(ECN_ECT1));
+  server_thread_->Resume();
   client_->SendSynchronousRequest("/foo");
   // A second request provides a packet for the client ACKs to go with.
   client_->SendSynchronousRequest("/foo");
-  server_thread_->Pause();
-  EXPECT_EQ(ecn->ect0, 0);
-  EXPECT_EQ(ecn->ce, 0);
-  if (!VersionHasIetfQuicFrames(version_.transport_version)) {
-    EXPECT_EQ(ecn->ect1, 0);
-  } else {
-    EXPECT_GT(ecn->ect1, 0);
-  }
-  server_connection->set_per_packet_options(nullptr);
-  server_thread_->Resume();
+
+  server_thread_->ScheduleAndWaitForCompletion([&] {
+    EXPECT_EQ(ecn->ce, 0);
+    if (!VersionHasIetfQuicFrames(version_.transport_version)) {
+      EXPECT_EQ(ecn->ect1, 0);
+    } else {
+      EXPECT_GT(ecn->ect1, 0);
+    }
+  });
+
   client_->Disconnect();
 }