test: hopefully deflaking echo integration test (envoyproxy#4304)

At least one failure mode is that when the listener was released, some other test would yoink the released port, and the "make sure we can not connect to a removed listener" check would unexpectedly result in a connection. Running the test as exclusive should fix that particular failure mode, and allow us to see if others exist.

I believe the reason the test was flaking more often when run in parallel with -l trace is because the test ran more slowly, the lag between the listener releasing the port and the raw connection driver increased, so the likelihood that another test would snag the port also increased.

Risk Level: Low (test only)
Testing: 1000 runs with "exclusive"
Docs Changes: n/a
Release Notes: n/a

Fixes envoyproxy#3997

Signed-off-by: Alyssa Wilk <alyssar@chromium.org>

test/integration/BUILD

@@ -344,6 +344,8 @@ envoy_cc_test(
     srcs = [
         "echo_integration_test.cc",
     ],
+    # This test must be run in exclusive mode: see comments in AddRemoveListener
+    tags = ["exclusive"],
     deps = [
         ":integration_lib",
         "//source/extensions/filters/network/echo:config",

test/integration/echo_integration_test.cc

@@ -118,10 +118,25 @@ TEST_P(EchoIntegrationTest, AddRemoveListener) {
   listener_removed.waitReady();
 
   // Now connect. This should fail.
-  RawConnectionDriver connection2(
-      new_listener_port, buffer,
-      [&](Network::ClientConnection&, const Buffer::Instance&) -> void { FAIL(); }, version_);
-  connection2.run();
+  // Allow for a few attempts, in order to handle a race (likely due to lack of
+  // LEV_OPT_CLOSE_ON_FREE, which would break listener reuse)
+  //
+  // In order for this test to work, it must be tagged as "exclusive" in its
+  // build file. Otherwise, it's possible that when the listener is destroyed
+  // above, another test would start listening on the released port, and this
+  // connect would unexpectedly succeed.
+  bool connect_fail = false;
+  for (int i = 0; i < 10; ++i) {
+    RawConnectionDriver connection2(
+        new_listener_port, buffer,
+        [&](Network::ClientConnection&, const Buffer::Instance&) -> void { FAIL(); }, version_);
+    connection2.run(Event::Dispatcher::RunType::NonBlock);
+    if (connection2.connection().state() == Network::Connection::State::Closed) {
+      connect_fail = true;
+      break;
+    }
+  }
+  ASSERT_TRUE(connect_fail);
 }
 
 } // namespace Envoy

test/integration/utility.cc

@@ -122,7 +122,7 @@ RawConnectionDriver::RawConnectionDriver(uint32_t port, Buffer::Instance& initia
 
 RawConnectionDriver::~RawConnectionDriver() {}
 
-void RawConnectionDriver::run() { dispatcher_->run(Event::Dispatcher::RunType::Block); }
+void RawConnectionDriver::run(Event::Dispatcher::RunType run_type) { dispatcher_->run(run_type); }
 
 void RawConnectionDriver::close() { client_->close(Network::ConnectionCloseType::FlushWrite); }
 

test/integration/utility.h

@@ -61,7 +61,8 @@ class RawConnectionDriver {
   RawConnectionDriver(uint32_t port, Buffer::Instance& initial_data, ReadCallback data_callback,
                       Network::Address::IpVersion version);
   ~RawConnectionDriver();
-  void run();
+  const Network::Connection& connection() { return *client_; }
+  void run(Event::Dispatcher::RunType run_type = Event::Dispatcher::RunType::Block);
   void close();
 
 private: