quic: fix test flake due to no socket interface (#35526)

Fixes #35467 The failure is not during an important part of the test, but we are receiving some packet while there is no socket interface. To workaround this, do an atomic swap of the interface pointer so there is never a time when we don't have a socket interface. Signed-off-by: Greg Greenway <[email protected]>
envoyproxy · Jul 31, 2024 · f7e5b59 · f7e5b59
1 parent 34bdb94
commit f7e5b59
Show file tree

Hide file tree

Showing 5 changed files with 20 additions and 29 deletions.
diff --git a/source/common/singleton/threadsafe_singleton.h b/source/common/singleton/threadsafe_singleton.h
@@ -66,6 +66,9 @@ template <class T> class InjectableSingleton {
   }
   static void clear() { loader_ = nullptr; }
 
+  // Atomically replace the value, returning the old value.
+  static T* replaceForTest(T* new_value) { return loader_.exchange(new_value); }
+
 protected:
   static std::atomic<T*> loader_;
 };
@@ -86,20 +89,16 @@ template <class T> class ScopedInjectableLoader {
   std::unique_ptr<T> instance_;
 };
 
-// This class saves the singleton object and restore the original singleton at destroy. This class
-// is not thread safe. It can be used in single thread test.
-template <class T>
-class StackedScopedInjectableLoader :
-    // To access the protected loader_.
-    protected InjectableSingleton<T> {
+// This class saves the singleton object and restore the original singleton at destroy.
+template <class T> class StackedScopedInjectableLoaderForTest {
 public:
-  explicit StackedScopedInjectableLoader(std::unique_ptr<T>&& instance) {
-    original_loader_ = InjectableSingleton<T>::getExisting();
-    InjectableSingleton<T>::clear();
+  explicit StackedScopedInjectableLoaderForTest(std::unique_ptr<T>&& instance) {
     instance_ = std::move(instance);
-    InjectableSingleton<T>::initialize(instance_.get());
+    original_loader_ = InjectableSingleton<T>::replaceForTest(instance_.get());
+  }
+  ~StackedScopedInjectableLoaderForTest() {
+    InjectableSingleton<T>::replaceForTest(original_loader_);
   }
-  ~StackedScopedInjectableLoader() { InjectableSingleton<T>::loader_ = original_loader_; }
 
 private:
   std::unique_ptr<T> instance_;

diff --git a/test/common/listener_manager/listener_manager_impl_test.cc b/test/common/listener_manager/listener_manager_impl_test.cc
@@ -2604,7 +2604,8 @@ TEST_P(ListenerManagerImplTest, BindToPortEqualToFalse) {
   InSequence s;
   auto mock_interface = std::make_unique<Network::MockSocketInterface>(
       std::vector<Network::Address::IpVersion>{Network::Address::IpVersion::v4});
-  StackedScopedInjectableLoader<Network::SocketInterface> new_interface(std::move(mock_interface));
+  StackedScopedInjectableLoaderForTest<Network::SocketInterface> new_interface(
+      std::move(mock_interface));
 
   ProdListenerComponentFactory real_listener_factory(server_);
   EXPECT_CALL(*worker_, start(_, _));
@@ -2643,7 +2644,8 @@ TEST_P(ListenerManagerImplTest, UpdateBindToPortEqualToFalse) {
   InSequence s;
   auto mock_interface = std::make_unique<Network::MockSocketInterface>(
       std::vector<Network::Address::IpVersion>{Network::Address::IpVersion::v4});
-  StackedScopedInjectableLoader<Network::SocketInterface> new_interface(std::move(mock_interface));
+  StackedScopedInjectableLoaderForTest<Network::SocketInterface> new_interface(
+      std::move(mock_interface));
 
   ProdListenerComponentFactory real_listener_factory(server_);
   EXPECT_CALL(*worker_, start(_, _));

diff --git a/test/common/network/listen_socket_impl_test.cc b/test/common/network/listen_socket_impl_test.cc
@@ -229,7 +229,7 @@ TEST_P(ListenSocketImplTestTcp, SupportedIpFamilyVirtualSocketIsCreatedWithNoBsd
   auto any_address = version_ == Address::IpVersion::v4 ? Utility::getIpv4AnyAddress()
                                                         : Utility::getIpv6AnyAddress();
 
-  StackedScopedInjectableLoader<SocketInterface> new_interface(std::move(mock_interface));
+  StackedScopedInjectableLoaderForTest<SocketInterface> new_interface(std::move(mock_interface));
 
   {
     EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0);
@@ -245,7 +245,7 @@ TEST_P(ListenSocketImplTestTcp, DeathAtUnSupportedIpFamilyListenSocket) {
   auto* mock_interface_ptr = mock_interface.get();
   auto the_other_address = version_ == Address::IpVersion::v4 ? Utility::getIpv6AnyAddress()
                                                               : Utility::getIpv4AnyAddress();
-  StackedScopedInjectableLoader<SocketInterface> new_interface(std::move(mock_interface));
+  StackedScopedInjectableLoaderForTest<SocketInterface> new_interface(std::move(mock_interface));
   {
     EXPECT_CALL(*mock_interface_ptr, socket(_, _, _)).Times(0);
     EXPECT_CALL(*mock_interface_ptr, socket(_, _, _, _, _)).Times(0);

diff --git a/test/integration/socket_interface_swap.cc b/test/integration/socket_interface_swap.cc
@@ -3,10 +3,8 @@
 namespace Envoy {
 
 SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type)
-    : write_matcher_(std::make_shared<IoHandleMatcher>(socket_type)) {
-  Envoy::Network::SocketInterfaceSingleton::clear();
-  test_socket_interface_loader_ = std::make_unique<Envoy::Network::SocketInterfaceLoader>(
-      std::make_unique<Envoy::Network::TestSocketInterface>(
+    : write_matcher_(std::make_shared<IoHandleMatcher>(socket_type)),
+      test_socket_interface_loader_(std::make_unique<Envoy::Network::TestSocketInterface>(
           [write_matcher = write_matcher_](Envoy::Network::TestIoSocketHandle* io_handle)
               -> absl::optional<Api::IoCallUint64Result> {
             Api::IoErrorPtr error_override = write_matcher->returnConnectOverride(io_handle);
@@ -28,8 +26,7 @@ SocketInterfaceSwap::SocketInterfaceSwap(Network::Socket::Type socket_type)
           },
           [write_matcher = write_matcher_](Network::IoHandle::RecvMsgOutput& output) {
             write_matcher->readOverride(output);
-          }));
-}
+          })) {}
 
 void SocketInterfaceSwap::IoHandleMatcher::setResumeWrites() {
   absl::MutexLock lock(&mutex_);

diff --git a/test/integration/socket_interface_swap.h b/test/integration/socket_interface_swap.h
@@ -116,15 +116,8 @@ class SocketInterfaceSwap {
 
   explicit SocketInterfaceSwap(Network::Socket::Type socket_type);
 
-  ~SocketInterfaceSwap() {
-    test_socket_interface_loader_.reset();
-    Envoy::Network::SocketInterfaceSingleton::initialize(previous_socket_interface_);
-  }
-
-  Envoy::Network::SocketInterface* const previous_socket_interface_{
-      Envoy::Network::SocketInterfaceSingleton::getExisting()};
   std::shared_ptr<IoHandleMatcher> write_matcher_;
-  std::unique_ptr<Envoy::Network::SocketInterfaceLoader> test_socket_interface_loader_;
+  StackedScopedInjectableLoaderForTest<Network::SocketInterface> test_socket_interface_loader_;
 };
 
 } // namespace Envoy