testing: fix multiple race conditions in simulated time tests (envoyp…

…roxy#12527) This PR fixes multiple race conditions in tests. The summary is: 1) All waitFor() operations are now fully synchronized. 2) waitFor() no longer moves simulated time and performs real sleeps in all time systems. This means that all network operations are now "instantaneous" and makes all time advances for alarms explicit. This required fixes in a few tests but should make simulated time much easier to reason about. 3) All timeout durations for network operations use real time for timeouts. Signed-off-by: Matt Klein <[email protected]>
hango-io · Aug 14, 2020 · a42a677 · a42a677
1 parent b09971d
commit a42a677
Show file tree

Hide file tree

Showing 42 changed files with 472 additions and 561 deletions.
diff --git a/test/common/formatter/substitution_formatter_fuzz_test.cc b/test/common/formatter/substitution_formatter_fuzz_test.cc
@@ -19,9 +19,9 @@ DEFINE_PROTO_FUZZER(const test::common::substitution::TestCase& input) {
         Fuzz::fromHeaders<Http::TestResponseHeaderMapImpl>(input.response_headers());
     const auto& response_trailers =
         Fuzz::fromHeaders<Http::TestResponseTrailerMapImpl>(input.response_trailers());
-    const auto& stream_info = Fuzz::fromStreamInfo(input.stream_info());
+    const std::unique_ptr<TestStreamInfo> stream_info = Fuzz::fromStreamInfo(input.stream_info());
     for (const auto& it : formatters) {
-      it->format(request_headers, response_headers, response_trailers, stream_info,
+      it->format(request_headers, response_headers, response_trailers, *stream_info,
                  absl::string_view());
     }
     ENVOY_LOG_MISC(trace, "Success");

diff --git a/test/common/router/header_parser_fuzz_test.cc b/test/common/router/header_parser_fuzz_test.cc
@@ -15,8 +15,8 @@ DEFINE_PROTO_FUZZER(const test::common::router::TestCase& input) {
     Router::HeaderParserPtr parser =
         Router::HeaderParser::configure(input.headers_to_add(), input.headers_to_remove());
     Http::TestRequestHeaderMapImpl header_map;
-    TestStreamInfo test_stream_info = fromStreamInfo(input.stream_info());
-    parser->evaluateHeaders(header_map, test_stream_info);
+    std::unique_ptr<TestStreamInfo> test_stream_info = fromStreamInfo(input.stream_info());
+    parser->evaluateHeaders(header_map, *test_stream_info);
     ENVOY_LOG_MISC(trace, "Success");
   } catch (const EnvoyException& e) {
     ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what());

diff --git a/test/extensions/filters/common/expr/evaluator_fuzz_test.cc b/test/extensions/filters/common/expr/evaluator_fuzz_test.cc
@@ -27,7 +27,7 @@ DEFINE_PROTO_FUZZER(const test::extensions::filters::common::expr::EvaluatorTest
     // Validate that the input has an expression.
     TestUtility::validate(input);
     // Create stream_info to test against, this may catch exceptions from invalid addresses.
-    stream_info = std::make_unique<TestStreamInfo>(Fuzz::fromStreamInfo(input.stream_info()));
+    stream_info = Fuzz::fromStreamInfo(input.stream_info());
   } catch (const EnvoyException& e) {
     ENVOY_LOG_MISC(debug, "EnvoyException: {}", e.what());
     return;

diff --git a/test/extensions/filters/http/fault/fault_filter_integration_test.cc b/test/extensions/filters/http/fault/fault_filter_integration_test.cc
@@ -125,15 +125,12 @@ TEST_P(FaultIntegrationTestAllProtocols, HeaderFaultConfig) {
                                                  {":authority", "host"},
                                                  {"x-envoy-fault-delay-request", "200"},
                                                  {"x-envoy-fault-throughput-response", "1"}};
-  const auto current_time = simTime().monotonicTime();
   IntegrationStreamDecoderPtr decoder = codec_client_->makeHeaderOnlyRequest(request_headers);
+  test_server_->waitForCounterEq("http.config_test.fault.delays_injected", 1,
+                                 TestUtility::DefaultTimeout, dispatcher_.get());
+  simTime().advanceTimeWait(std::chrono::milliseconds(200));
   waitForNextUpstreamRequest();
 
-  // At least 200ms of simulated time should have elapsed before we got the upstream request.
-  EXPECT_LE(std::chrono::milliseconds(200), simTime().monotonicTime() - current_time);
-  // Active faults gauge is incremented.
-  EXPECT_EQ(1UL, test_server_->gauge("http.config_test.fault.active_faults")->value());
-
   // Verify response body throttling.
   upstream_request_->encodeHeaders(default_response_headers_, false);
   Buffer::OwnedImpl data(std::string(128, 'a'));
@@ -215,6 +212,9 @@ TEST_P(FaultIntegrationTestAllProtocols, HeaderFaultsConfig100PercentageHeaders)
                                      {"x-envoy-fault-delay-request-percentage", "100"},
                                      {"x-envoy-fault-throughput-response", "100"},
                                      {"x-envoy-fault-throughput-response-percentage", "100"}});
+  test_server_->waitForCounterEq("http.config_test.fault.delays_injected", 1,
+                                 TestUtility::DefaultTimeout, dispatcher_.get());
+  simTime().advanceTimeWait(std::chrono::milliseconds(100));
   waitForNextUpstreamRequest();
   upstream_request_->encodeHeaders(default_response_headers_, true);
   response->waitForEndStream();

diff --git a/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc b/test/extensions/filters/http/ratelimit/ratelimit_integration_test.cc
@@ -333,7 +333,7 @@ TEST_P(RatelimitIntegrationTest, ConnectImmediateDisconnect) {
   initiateClientConnection();
   ASSERT_TRUE(fake_upstreams_[1]->waitForHttpConnection(*dispatcher_, fake_ratelimit_connection_));
   ASSERT_TRUE(fake_ratelimit_connection_->close());
-  ASSERT_TRUE(fake_ratelimit_connection_->waitForDisconnect(true));
+  ASSERT_TRUE(fake_ratelimit_connection_->waitForDisconnect());
   fake_ratelimit_connection_ = nullptr;
   // Rate limiter fails open
   waitForSuccessfulUpstreamResponse();

diff --git a/test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc b/test/extensions/filters/network/local_ratelimit/local_ratelimit_integration_test.cc
@@ -40,7 +40,7 @@ name: ratelimit
   ASSERT_TRUE(fake_upstream_connection->write("world"));
   tcp_client->waitForData("world");
   tcp_client->close();
-  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect(true));
+  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
 
   EXPECT_EQ(0,
             test_server_->counter("local_rate_limit.local_rate_limit_stats.rate_limited")->value());

diff --git a/test/fuzz/utility.h b/test/fuzz/utility.h
@@ -128,15 +128,15 @@ inline test::fuzz::Headers toHeaders(const Http::HeaderMap& headers) {
 const std::string TestSubjectPeer =
     "CN=Test Server,OU=Lyft Engineering,O=Lyft,L=San Francisco,ST=California,C=US";
 
-inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info) {
+inline std::unique_ptr<TestStreamInfo> fromStreamInfo(const test::fuzz::StreamInfo& stream_info) {
   // Set mocks' default string return value to be an empty string.
   // TODO(asraa): Speed up this function, which is slowed because of the use of mocks.
   testing::DefaultValue<const std::string&>::Set(EMPTY_STRING);
-  TestStreamInfo test_stream_info;
-  test_stream_info.metadata_ = stream_info.dynamic_metadata();
+  auto test_stream_info = std::make_unique<TestStreamInfo>();
+  test_stream_info->metadata_ = stream_info.dynamic_metadata();
   // Truncate recursive filter metadata fields.
   // TODO(asraa): Resolve MessageToJsonString failure on recursive filter metadata.
-  for (auto& pair : *test_stream_info.metadata_.mutable_filter_metadata()) {
+  for (auto& pair : *test_stream_info->metadata_.mutable_filter_metadata()) {
     std::string value;
     pair.second.SerializeToString(&value);
     pair.second.ParseFromString(value.substr(0, 128));
@@ -147,31 +147,31 @@ inline TestStreamInfo fromStreamInfo(const test::fuzz::StreamInfo& stream_info)
               stream_info.start_time()
           ? 0
           : stream_info.start_time() / 1000;
-  test_stream_info.start_time_ = SystemTime(std::chrono::microseconds(start_time));
+  test_stream_info->start_time_ = SystemTime(std::chrono::microseconds(start_time));
   if (stream_info.has_response_code()) {
-    test_stream_info.response_code_ = stream_info.response_code().value();
+    test_stream_info->response_code_ = stream_info.response_code().value();
   }
-  test_stream_info.setRequestedServerName(stream_info.requested_server_name());
+  test_stream_info->setRequestedServerName(stream_info.requested_server_name());
   auto upstream_host = std::make_shared<NiceMock<Upstream::MockHostDescription>>();
   auto upstream_metadata = std::make_shared<envoy::config::core::v3::Metadata>(
       replaceInvalidStringValues(stream_info.upstream_metadata()));
   ON_CALL(*upstream_host, metadata()).WillByDefault(testing::Return(upstream_metadata));
-  test_stream_info.upstream_host_ = upstream_host;
+  test_stream_info->upstream_host_ = upstream_host;
   auto address = stream_info.has_address()
                      ? Envoy::Network::Address::resolveProtoAddress(stream_info.address())
                      : Network::Utility::resolveUrl("tcp://10.0.0.1:443");
   auto upstream_local_address =
       stream_info.has_upstream_local_address()
           ? Envoy::Network::Address::resolveProtoAddress(stream_info.upstream_local_address())
           : Network::Utility::resolveUrl("tcp://10.0.0.1:10000");
-  test_stream_info.upstream_local_address_ = upstream_local_address;
-  test_stream_info.downstream_local_address_ = address;
-  test_stream_info.downstream_direct_remote_address_ = address;
-  test_stream_info.downstream_remote_address_ = address;
+  test_stream_info->upstream_local_address_ = upstream_local_address;
+  test_stream_info->downstream_local_address_ = address;
+  test_stream_info->downstream_direct_remote_address_ = address;
+  test_stream_info->downstream_remote_address_ = address;
   auto connection_info = std::make_shared<NiceMock<Ssl::MockConnectionInfo>>();
   ON_CALL(*connection_info, subjectPeerCertificate())
       .WillByDefault(testing::ReturnRef(TestSubjectPeer));
-  test_stream_info.setDownstreamSslConnection(connection_info);
+  test_stream_info->setDownstreamSslConnection(connection_info);
   return test_stream_info;
 }
 

diff --git a/test/integration/BUILD b/test/integration/BUILD
@@ -896,7 +896,10 @@ envoy_cc_test(
 envoy_cc_test_library(
     name = "server_stats_interface",
     hdrs = ["server_stats.h"],
-    deps = ["//include/envoy/stats:stats_interface"],
+    deps = [
+        "//include/envoy/event:dispatcher_interface",
+        "//include/envoy/stats:stats_interface",
+    ],
 )
 
 envoy_cc_test(

diff --git a/test/integration/autonomous_upstream.cc b/test/integration/autonomous_upstream.cc
@@ -48,7 +48,7 @@ void AutonomousStream::sendResponse() {
   int32_t request_body_length = -1;
   HeaderToInt(EXPECT_REQUEST_SIZE_BYTES, request_body_length, headers);
   if (request_body_length >= 0) {
-    EXPECT_EQ(request_body_length, bodyLength());
+    EXPECT_EQ(request_body_length, body_.length());
   }
 
   if (!headers.get_(RESET_AFTER_REQUEST).empty()) {

diff --git a/test/integration/autonomous_upstream.h b/test/integration/autonomous_upstream.h
@@ -24,11 +24,11 @@ class AutonomousStream : public FakeStream {
                    AutonomousUpstream& upstream, bool allow_incomplete_streams);
   ~AutonomousStream() override;
 
-  void setEndStream(bool set) override;
+  void setEndStream(bool set) EXCLUSIVE_LOCKS_REQUIRED(lock_) override;
 
 private:
   AutonomousUpstream& upstream_;
-  void sendResponse();
+  void sendResponse() EXCLUSIVE_LOCKS_REQUIRED(lock_);
   const bool allow_incomplete_streams_{false};
 };
 

diff --git a/test/integration/cluster_filter_integration_test.cc b/test/integration/cluster_filter_integration_test.cc
@@ -124,7 +124,7 @@ TEST_P(ClusterFilterIntegrationTest, TestClusterFilter) {
   ASSERT_TRUE(tcp_client->write("", true));
   ASSERT_TRUE(fake_upstream_connection->waitForHalfClose());
   ASSERT_TRUE(fake_upstream_connection->write("", true));
-  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect(true));
+  ASSERT_TRUE(fake_upstream_connection->waitForDisconnect());
   tcp_client->waitForDisconnect();
 }
 

diff --git a/test/integration/cx_limit_integration_test.cc b/test/integration/cx_limit_integration_test.cc
@@ -47,7 +47,8 @@ class ConnectionLimitIntegrationTest : public testing::TestWithParam<Network::Ad
       if (Network::AcceptedSocketImpl::acceptedSocketCount() == expected_connections) {
         return AssertionSuccess();
       }
-      timeSystem().advanceTimeWait(std::chrono::milliseconds(500));
+      // TODO(mattklein123): Do not use a real sleep here. Switch to events with waitFor().
+      timeSystem().realSleepDoNotUseWithoutScrutiny(std::chrono::milliseconds(500));
     }
     if (Network::AcceptedSocketImpl::acceptedSocketCount() == expected_connections) {
       return AssertionSuccess();