From 5b48b13dc163115cf2a828bfd9e1f75d8496677d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 08:10:23 -0400 Subject: [PATCH 1/6] use SimulatedTimeSystem rather than MockTimeSystem in all but two places. Signed-off-by: Joshua Marantz --- test/common/config/BUILD | 1 + test/common/config/grpc_mux_impl_test.cc | 16 +++++++++-- test/common/http/BUILD | 2 +- .../http/conn_manager_impl_fuzz_test.cc | 9 +++--- test/common/network/BUILD | 2 +- test/common/upstream/BUILD | 1 + .../upstream/load_stats_reporter_test.cc | 28 ++++++------------- .../stats_sinks/metrics_service/BUILD | 1 + .../grpc_metrics_service_impl_test.cc | 9 +++--- test/test_common/simulated_time_system.cc | 1 + 10 files changed, 37 insertions(+), 33 deletions(-) diff --git a/test/common/config/BUILD b/test/common/config/BUILD index 9b1756a2e9386..d97c5b3daf92b 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -52,6 +52,7 @@ envoy_cc_test( "//test/mocks/local_info:local_info_mocks", "//test/mocks/runtime:runtime_mocks", "//test/test_common:logging_lib", + "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:discovery_cc", "@envoy_api//envoy/api/v2:eds_cc", diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index 030e7dbe26ec2..fb1abfb5c34fc 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -14,6 +14,7 @@ #include "test/mocks/local_info/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/test_common/logging.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -36,7 +37,7 @@ namespace { class GrpcMuxImplTest : public testing::Test { public: GrpcMuxImplTest() : async_client_(new Grpc::MockAsyncClient()) { - dispatcher_.setTimeSystem(mock_time_system_); + dispatcher_.setTimeSystem(time_system_); } void setup() { @@ -76,7 +77,7 @@ class GrpcMuxImplTest : public testing::Test { Grpc::MockAsyncStream async_stream_; std::unique_ptr grpc_mux_; NiceMock callbacks_; - NiceMock mock_time_system_; + Event::SimulatedTimeSystem time_system_; NiceMock local_info_; Stats::IsolatedStoreImpl stats_; }; @@ -310,8 +311,17 @@ TEST_F(GrpcMuxImplTest, WatchDemux) { expectSendMessage(type_url, {}, "2"); } +// Exactly one test requires a mock time system to provoke behavior that cannot +// easily be achieved with a SimulatedTimeSystem. +class GrpcMuxImplTestWithMockTimeSystem : public GrpcMuxImplTest { +protected: + GrpcMuxImplTestWithMockTimeSystem() { dispatcher_.setTimeSystem(mock_time_system_); } + + MockTimeSystem mock_time_system_; +}; + // Verifies that warning messages get logged when Envoy detects too many requests. -TEST_F(GrpcMuxImplTest, TooManyRequests) { +TEST_F(GrpcMuxImplTestWithMockTimeSystem, TooManyRequests) { setup(); EXPECT_CALL(async_stream_, sendMessage(_, false)).Times(AtLeast(100)); diff --git a/test/common/http/BUILD b/test/common/http/BUILD index 93fa583a5c478..b9f6104ed30e5 100644 --- a/test/common/http/BUILD +++ b/test/common/http/BUILD @@ -121,7 +121,7 @@ envoy_cc_fuzz_test( "//test/mocks/ssl:ssl_mocks", "//test/mocks/tracing:tracing_mocks", "//test/mocks/upstream:upstream_mocks", - "//test/test_common:test_time_lib", + "//test/test_common:simulated_time_system_lib", "@envoy_api//envoy/config/filter/network/http_connection_manager/v2:http_connection_manager_cc", ], ) diff --git a/test/common/http/conn_manager_impl_fuzz_test.cc b/test/common/http/conn_manager_impl_fuzz_test.cc index 4144b141ef8af..7fb359966cdf0 100644 --- a/test/common/http/conn_manager_impl_fuzz_test.cc +++ b/test/common/http/conn_manager_impl_fuzz_test.cc @@ -31,7 +31,7 @@ #include "test/mocks/ssl/mocks.h" #include "test/mocks/tracing/mocks.h" #include "test/mocks/upstream/mocks.h" -#include "test/test_common/test_time.h" +#include "test/test_common/simulated_time_system.h" #include "gmock/gmock.h" @@ -56,7 +56,7 @@ class FuzzConfig : public ConnectionManagerConfig { }; FuzzConfig() - : route_config_provider_(test_time_.timeSystem()), + : route_config_provider_(time_system_), stats_{{ALL_HTTP_CONN_MAN_STATS(POOL_COUNTER(fake_stats_), POOL_GAUGE(fake_stats_), POOL_HISTOGRAM(fake_stats_))}, "", @@ -122,7 +122,7 @@ class FuzzConfig : public ConnectionManagerConfig { MockStreamEncoderFilter* encoder_filter_{}; NiceMock filter_factory_; absl::optional idle_timeout_; - DangerousDeprecatedTestTime test_time_; + Event::SimulatedTimeSystem time_system_; RouteConfigProvider route_config_provider_; std::string server_name_; Stats::IsolatedStoreImpl fake_stats_; @@ -384,7 +384,6 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { NiceMock local_info; NiceMock cluster_manager; NiceMock filter_callbacks; - NiceMock time_system; std::unique_ptr ssl_connection; bool connection_alive = true; @@ -398,7 +397,7 @@ DEFINE_PROTO_FUZZER(const test::common::http::ConnManagerImplTestCase& input) { std::make_shared("0.0.0.0"); ConnectionManagerImpl conn_manager(config, drain_close, random, tracer, runtime, local_info, - cluster_manager, nullptr, time_system); + cluster_manager, nullptr, config.time_system_); conn_manager.initializeReadFilterCallbacks(filter_callbacks); std::vector streams; diff --git a/test/common/network/BUILD b/test/common/network/BUILD index ba4eadec52082..23b5f1548ccda 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -51,7 +51,7 @@ envoy_cc_test( "//test/mocks/stats:stats_mocks", "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", - "//test/test_common:test_time_lib", + "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/common/upstream/BUILD b/test/common/upstream/BUILD index a308e0881b0ba..789582d242f6f 100644 --- a/test/common/upstream/BUILD +++ b/test/common/upstream/BUILD @@ -158,6 +158,7 @@ envoy_cc_test( "//test/mocks/grpc:grpc_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:simulated_time_system_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:eds_cc", "@envoy_api//envoy/api/v2/endpoint:endpoint_cc", diff --git a/test/common/upstream/load_stats_reporter_test.cc b/test/common/upstream/load_stats_reporter_test.cc index 1b69bfb2771a6..86bfaa79cb661 100644 --- a/test/common/upstream/load_stats_reporter_test.cc +++ b/test/common/upstream/load_stats_reporter_test.cc @@ -7,6 +7,7 @@ #include "test/mocks/grpc/mocks.h" #include "test/mocks/local_info/mocks.h" #include "test/mocks/upstream/mocks.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -75,7 +76,7 @@ class LoadStatsReporterTest : public testing::Test { Event::TimerCb response_timer_cb_; Grpc::MockAsyncStream async_stream_; Grpc::MockAsyncClient* async_client_; - MockTimeSystem time_system_; + Event::SimulatedTimeSystem time_system_; NiceMock local_info_; }; @@ -93,14 +94,12 @@ TEST_F(LoadStatsReporterTest, TestPubSub) { EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); EXPECT_CALL(async_stream_, sendMessage(_, _)); createLoadStatsReporter(); - EXPECT_CALL(time_system_, monotonicTime()); deliverLoadStatsResponse({"foo"}); EXPECT_CALL(async_stream_, sendMessage(_, _)); EXPECT_CALL(*response_timer_, enableTimer(std::chrono::milliseconds(42000))); response_timer_cb_(); - EXPECT_CALL(time_system_, monotonicTime()); deliverLoadStatsResponse({"bar"}); EXPECT_CALL(async_stream_, sendMessage(_, _)); @@ -114,8 +113,7 @@ TEST_F(LoadStatsReporterTest, ExistingClusters) { // Initially, we have no clusters to report on. expectSendMessage({}); createLoadStatsReporter(); - EXPECT_CALL(time_system_, monotonicTime()) - .WillOnce(Return(MonotonicTime(std::chrono::microseconds(3)))); + time_system_.setMonotonicTime(std::chrono::microseconds(3)); // Start reporting on foo. NiceMock foo_cluster; foo_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(2); @@ -125,8 +123,7 @@ TEST_F(LoadStatsReporterTest, ExistingClusters) { deliverLoadStatsResponse({"foo"}); // Initial stats report for foo on timer tick. foo_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(5); - EXPECT_CALL(time_system_, monotonicTime()) - .WillOnce(Return(MonotonicTime(std::chrono::microseconds(4)))); + time_system_.setMonotonicTime(std::chrono::microseconds(4)); { envoy::api::v2::endpoint::ClusterStats foo_cluster_stats; foo_cluster_stats.set_cluster_name("foo"); @@ -143,15 +140,12 @@ TEST_F(LoadStatsReporterTest, ExistingClusters) { bar_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); // Start reporting on bar. - EXPECT_CALL(time_system_, monotonicTime()) - .WillOnce(Return(MonotonicTime(std::chrono::microseconds(6)))); + time_system_.setMonotonicTime(std::chrono::microseconds(6)); deliverLoadStatsResponse({"foo", "bar"}); // Stats report foo/bar on timer tick. foo_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); bar_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); - EXPECT_CALL(time_system_, monotonicTime()) - .Times(2) - .WillRepeatedly(Return(MonotonicTime(std::chrono::microseconds(28)))); + time_system_.setMonotonicTime(std::chrono::microseconds(28)); { envoy::api::v2::endpoint::ClusterStats foo_cluster_stats; foo_cluster_stats.set_cluster_name("foo"); @@ -177,8 +171,7 @@ TEST_F(LoadStatsReporterTest, ExistingClusters) { // Stats report for bar on timer tick. foo_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(5); bar_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(5); - EXPECT_CALL(time_system_, monotonicTime()) - .WillOnce(Return(MonotonicTime(std::chrono::microseconds(33)))); + time_system_.setMonotonicTime(std::chrono::microseconds(33)); { envoy::api::v2::endpoint::ClusterStats bar_cluster_stats; bar_cluster_stats.set_cluster_name("bar"); @@ -195,15 +188,12 @@ TEST_F(LoadStatsReporterTest, ExistingClusters) { bar_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); // Start tracking foo again, we should forget earlier history for foo. - EXPECT_CALL(time_system_, monotonicTime()) - .WillOnce(Return(MonotonicTime(std::chrono::microseconds(43)))); + time_system_.setMonotonicTime(std::chrono::microseconds(43)); deliverLoadStatsResponse({"foo", "bar"}); // Stats report foo/bar on timer tick. foo_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); bar_cluster.info_->load_report_stats_.upstream_rq_dropped_.add(1); - EXPECT_CALL(time_system_, monotonicTime()) - .Times(2) - .WillRepeatedly(Return(MonotonicTime(std::chrono::microseconds(47)))); + time_system_.setMonotonicTime(std::chrono::microseconds(47)); { envoy::api::v2::endpoint::ClusterStats foo_cluster_stats; foo_cluster_stats.set_cluster_name("foo"); diff --git a/test/extensions/stats_sinks/metrics_service/BUILD b/test/extensions/stats_sinks/metrics_service/BUILD index c6b8050fa0e8e..d9759e8df43d6 100644 --- a/test/extensions/stats_sinks/metrics_service/BUILD +++ b/test/extensions/stats_sinks/metrics_service/BUILD @@ -25,6 +25,7 @@ envoy_extension_cc_test( "//test/mocks/local_info:local_info_mocks", "//test/mocks/thread_local:thread_local_mocks", "//test/mocks/upstream:upstream_mocks", + "//test/test_common:simulated_time_system_lib", ], ) diff --git a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc index ab078fe6953f1..861f14772afb6 100644 --- a/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc +++ b/test/extensions/stats_sinks/metrics_service/grpc_metrics_service_impl_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/local_info/mocks.h" #include "test/mocks/stats/mocks.h" #include "test/mocks/thread_local/mocks.h" +#include "test/test_common/simulated_time_system.h" using namespace std::chrono_literals; using testing::_; @@ -99,10 +100,10 @@ class MetricsServiceSinkTest : public testing::Test {}; TEST(MetricsServiceSinkTest, CheckSendCall) { NiceMock source; - NiceMock mock_time; + Event::SimulatedTimeSystem time_system; std::shared_ptr streamer_{new MockGrpcMetricsStreamer()}; - MetricsServiceSink sink(streamer_, mock_time); + MetricsServiceSink sink(streamer_, time_system); auto counter = std::make_shared>(); counter->name_ = "test_counter"; @@ -127,10 +128,10 @@ TEST(MetricsServiceSinkTest, CheckSendCall) { TEST(MetricsServiceSinkTest, CheckStatsCount) { NiceMock source; - NiceMock mock_time; + Event::SimulatedTimeSystem time_system; std::shared_ptr streamer_{new TestGrpcMetricsStreamer()}; - MetricsServiceSink sink(streamer_, mock_time); + MetricsServiceSink sink(streamer_, time_system); auto counter = std::make_shared>(); counter->name_ = "test_counter"; diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index 99df74a1cd7a2..3f10036f31834 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -140,6 +140,7 @@ SystemTime SimulatedTimeSystem::systemTime() { MonotonicTime SimulatedTimeSystem::monotonicTime() { Thread::LockGuard lock(mutex_); + std::cerr << "monotonic_time_=" << monotonic_time_.time_since_epoch().count() << std::endl; return monotonic_time_; } From eac0c69de240a0eab5717ee9ea5ca4438cd1aebe Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 08:26:00 -0400 Subject: [PATCH 2/6] Also include all easily convertible ConnectinImpl tests; one needed to stay a mock. Signed-off-by: Joshua Marantz --- test/common/network/connection_impl_test.cc | 30 +++++++++++++++------ 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 24a21b8f45a7f..d6501521d1647 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -20,7 +20,7 @@ #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" #include "test/test_common/printers.h" -#include "test/test_common/test_time.h" +#include "test/test_common/simulated_time_system.h" #include "test/test_common/utility.h" #include "gmock/gmock.h" @@ -77,7 +77,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest, TestUtility::ipTestParamsToString); TEST_P(ConnectionImplDeathTest, BadFd) { - MockTimeSystem time_system; + Event::SimulatedTimeSystem time_system; Event::DispatcherImpl dispatcher(time_system); EXPECT_DEATH_LOG_TO_STDERR( ConnectionImpl(dispatcher, std::make_unique(-1, nullptr, nullptr), @@ -89,7 +89,7 @@ class ConnectionImplTest : public testing::TestWithParam { public: void setUpBasicConnection() { if (dispatcher_.get() == nullptr) { - dispatcher_.reset(new Event::DispatcherImpl(time_system_)); + dispatcher_.reset(new Event::DispatcherImpl(timeSystem())); } listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); @@ -152,7 +152,7 @@ class ConnectionImplTest : public testing::TestWithParam { MockBufferFactory* factory = new StrictMock; dispatcher_.reset( - new Event::DispatcherImpl(time_system_, Buffer::WatermarkFactoryPtr{factory})); + new Event::DispatcherImpl(timeSystem(), Buffer::WatermarkFactoryPtr{factory})); // The first call to create a client session will get a MockBuffer. // Other calls for server sessions will by default get a normal OwnedImpl. EXPECT_CALL(*factory, create_(_, _)) @@ -168,8 +168,10 @@ class ConnectionImplTest : public testing::TestWithParam { })); } + virtual Event::TimeSystem& timeSystem() { return time_system_; } + protected: - MockTimeSystem time_system_; + Event::SimulatedTimeSystem time_system_; Event::DispatcherPtr dispatcher_; Stats::IsolatedStoreImpl stats_store_; Network::TcpListenSocket socket_{Network::Test::getAnyAddress(GetParam()), nullptr, true}; @@ -1006,9 +1008,21 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { dispatcher_->run(Event::Dispatcher::RunType::Block); } +// Exactly one test requires a mock time system to provoke behavior that cannot +// easily be achieved with a SimulatedTimeSystem. +class ConnectionImplWithMockTimeSystemTest : public ConnectionImplTest { + protected: + Event::TimeSystem& timeSystem() override { return mock_time_system_; } + + MockTimeSystem mock_time_system_; +}; +INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplWithMockTimeSystemTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + // Test that a FlushWriteAndDelay close triggers a timeout which forces Envoy to close the // connection when a client has not issued a close within the configured interval. -TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTimerTriggerTest) { +TEST_P(ConnectionImplWithMockTimeSystemTest, FlushWriteAndDelayCloseTimerTriggerTest) { setUpBasicConnection(); connect(); // This timer should always trigger since the client connection does not issue a close() during @@ -1469,7 +1483,7 @@ TEST_P(ReadBufferLimitTest, SomeLimit) { class TcpClientConnectionImplTest : public testing::TestWithParam { protected: TcpClientConnectionImplTest() : dispatcher_(time_system_) {} - MockTimeSystem time_system_; + Event::SimulatedTimeSystem time_system_; Event::DispatcherImpl dispatcher_; }; INSTANTIATE_TEST_CASE_P(IpVersions, TcpClientConnectionImplTest, @@ -1510,7 +1524,7 @@ TEST_P(TcpClientConnectionImplTest, BadConnectConnRefused) { class PipeClientConnectionImplTest : public testing::Test { protected: PipeClientConnectionImplTest() : dispatcher_(time_system_) {} - MockTimeSystem time_system_; + Event::SimulatedTimeSystem time_system_; Event::DispatcherImpl dispatcher_; const std::string path_{TestEnvironment::unixDomainSocketPath("foo")}; }; From 52e3d845ec211987e1f9ac01056401be40fe5d7d Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 08:28:46 -0400 Subject: [PATCH 3/6] format Signed-off-by: Joshua Marantz --- test/common/network/connection_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index d6501521d1647..3cba93231bc54 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -1011,7 +1011,7 @@ TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { // Exactly one test requires a mock time system to provoke behavior that cannot // easily be achieved with a SimulatedTimeSystem. class ConnectionImplWithMockTimeSystemTest : public ConnectionImplTest { - protected: +protected: Event::TimeSystem& timeSystem() override { return mock_time_system_; } MockTimeSystem mock_time_system_; From 3bb5c1431cd801611fe2e795f7ea3366ccd7f2a7 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 08:49:12 -0400 Subject: [PATCH 4/6] Remove the reverted parts from #4581 Signed-off-by: Joshua Marantz --- test/common/network/connection_impl_test.cc | 194 -------------------- 1 file changed, 194 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index b8ad431c2642a..04ed5091986da 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -872,200 +872,6 @@ TEST_P(ConnectionImplTest, EmptyReadOnCloseTest) { disconnect(true); } -// Test that a FlushWrite close immediately triggers a close after the write buffer is flushed. -TEST_P(ConnectionImplTest, FlushWriteCloseTest) { - setUpBasicConnection(); - connect(); - // Set a very high timeout value to prevent flaking. We are testing the common case where the - // timeout does not trigger. - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50000)); - - std::shared_ptr client_read_filter(new NiceMock()); - client_connection_->addReadFilter(client_read_filter); - - NiceMockConnectionStats stats; - server_connection_->setConnectionStats(stats.toBufferStats()); - - Buffer::OwnedImpl data("data"); - server_connection_->write(data, false); - - // Server connection flushes the write and immediately closes the socket. - // There shouldn't be a read/close race here (see issue #2929), since the client is blocked on - // reading and the connection should close gracefully via FIN. - EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("data"), false)) - .Times(1) - .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { - dispatcher_->exit(); - return FilterStatus::StopIteration; - })); - EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); - EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)).Times(1); - EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); - server_connection_->close(ConnectionCloseType::FlushWrite); - dispatcher_->run(Event::Dispatcher::RunType::Block); -} - -// Test that a FlushWrite close will trigger a timeout which closes the connection when the write -// buffer is not flushed within the configured interval. -TEST_P(ConnectionImplTest, FlushWriteCloseTimeoutTest) { - NiceMock dispatcher; - EXPECT_CALL(dispatcher.buffer_factory_, create_(_, _)) - .WillRepeatedly(Invoke([](std::function below_low, - std::function above_high) -> Buffer::Instance* { - // ConnectionImpl calls Envoy::MockBufferFactory::create(), which calls create_() and wraps - // the returned raw pointer below with a unique_ptr. - return new Buffer::WatermarkBuffer(below_low, above_high); - })); - - // This timer will be returned (transferring ownership) to the ConnectionImpl when createTimer() - // is called to allocate the delayed close timer. - auto timer = new Event::MockTimer(&dispatcher); - EXPECT_CALL(*timer, enableTimer(_)).Times(1); - EXPECT_CALL(*timer, disableTimer()).Times(1); - - auto file_event = std::make_unique>(); - EXPECT_CALL(dispatcher, createFileEvent_(0, _, _, _)).WillOnce(Return(file_event.release())); - - auto transport_socket = std::make_unique>(); - EXPECT_CALL(*transport_socket, canFlushClose()).WillOnce(Return(true)); - - auto server_connection = std::make_unique( - dispatcher, std::make_unique(0, nullptr, nullptr), - std::move(transport_socket), true); - - // Enable delayed connection close processing by setting a non-zero timeout value. The actual - // value (> 0) doesn't matter since the callback is triggered below. - server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(100)); - - NiceMockConnectionStats stats; - server_connection->setConnectionStats(stats.toBufferStats()); - EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(1); - - Buffer::OwnedImpl data("data"); - server_connection->write(data, false); - // Data is pending in the write buffer, which will trigger the FlushWrite close to go into delayed - // close processing. - server_connection->close(ConnectionCloseType::FlushWrite); - - // Issue the delayed close callback to ensure connection is closed. - timer->callback_(); -} - -// Test that a FlushWriteAndDelay close causes Envoy to flush the write and wait for the client/peer -// to close (until a configured timeout which is not expected to trigger in this test). -TEST_P(ConnectionImplTest, FlushWriteAndDelayCloseTest) { -#ifdef __APPLE__ - // libevent does not provide early close notifications on the currently supported macOS builds, so - // the server connection is never notified of the close. For now, we have chosen to disable tests - // that rely on this behavior on macOS (see https://github.com/envoyproxy/envoy/pull/4299). - return; -#endif - setUpBasicConnection(); - connect(); - // Set a very high timeout value to prevent flaking. We are testing the common case where the - // timeout does not trigger. - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50000)); - - std::shared_ptr client_read_filter(new NiceMock()); - client_connection_->addReadFilter(client_read_filter); - - NiceMockConnectionStats stats; - server_connection_->setConnectionStats(stats.toBufferStats()); - - Buffer::OwnedImpl data("Connection: Close"); - server_connection_->write(data, false); - - EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("Connection: Close"), false)) - .Times(1) - .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { - client_connection_->close(ConnectionCloseType::NoFlush); - return FilterStatus::StopIteration; - })); - - // Client closes the connection so delayed close timer on the server conn should not fire. - EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); - EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); - EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::RemoteClose)) - .Times(1) - .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); - server_connection_->close(ConnectionCloseType::FlushWriteAndDelay); - dispatcher_->run(Event::Dispatcher::RunType::Block); -} - -// Exactly one test requires a mock time system to provoke behavior that cannot -// easily be achieved with a SimulatedTimeSystem. -class ConnectionImplWithMockTimeSystemTest : public ConnectionImplTest { -protected: - Event::TimeSystem& timeSystem() override { return mock_time_system_; } - - MockTimeSystem mock_time_system_; -}; -INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplWithMockTimeSystemTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); - -// Test that a FlushWriteAndDelay close triggers a timeout which forces Envoy to close the -// connection when a client has not issued a close within the configured interval. -TEST_P(ConnectionImplWithMockTimeSystemTest, FlushWriteAndDelayCloseTimerTriggerTest) { - setUpBasicConnection(); - connect(); - // This timer should always trigger since the client connection does not issue a close() during - // this test. - server_connection_->setDelayedCloseTimeout(std::chrono::milliseconds(50)); - - std::shared_ptr client_read_filter(new NiceMock()); - client_connection_->addReadFilter(client_read_filter); - - NiceMockConnectionStats stats; - server_connection_->setConnectionStats(stats.toBufferStats()); - - Buffer::OwnedImpl data("Connection: Close"); - server_connection_->write(data, false); - - // The client _will not_ close the connection. Instead, expect the delayed close timer to trigger - // on the server connection. - EXPECT_CALL(*client_read_filter, onData(BufferStringEqual("Connection: Close"), false)) - .Times(1) - .WillOnce(InvokeWithoutArgs([&]() -> FilterStatus { return FilterStatus::StopIteration; })); - EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(1); - EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) - .Times(1) - .WillOnce(Invoke([&](Network::ConnectionEvent) -> void { dispatcher_->exit(); })); - EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).Times(1); - server_connection_->close(ConnectionCloseType::FlushWriteAndDelay); - dispatcher_->run(Event::Dispatcher::RunType::Block); -} - -// Test that delayed close processing can be disabled by setting the delayed close timeout interval -// to 0. -TEST_P(ConnectionImplTest, FlushWriteAndDelayConfigDisabledTest) { - NiceMock callbacks; - NiceMock dispatcher; - EXPECT_CALL(dispatcher.buffer_factory_, create_(_, _)) - .WillRepeatedly(Invoke([](std::function below_low, - std::function above_high) -> Buffer::Instance* { - return new Buffer::WatermarkBuffer(below_low, above_high); - })); - std::unique_ptr server_connection(new Network::ConnectionImpl( - dispatcher, std::make_unique(0, nullptr, nullptr), - std::make_unique>(), true)); - - // Ensure the delayed close timer is not created when the delayedCloseTimeout config value is set - // to 0. - server_connection->setDelayedCloseTimeout(std::chrono::milliseconds(0)); - EXPECT_CALL(dispatcher, createTimer_(_)).Times(0); - - NiceMockConnectionStats stats; - server_connection->setConnectionStats(stats.toBufferStats()); - - EXPECT_CALL(stats.delayed_close_timeouts_, inc()).Times(0); - server_connection->close(ConnectionCloseType::FlushWriteAndDelay); - - // Since the delayed close timer never triggers, the connection never closes. Close it here to end - // the test cleanly due to the (fd == -1) assert in ~ConnectionImpl(). - server_connection->close(ConnectionCloseType::NoFlush); -} - class MockTransportConnectionImplTest : public testing::Test { public: MockTransportConnectionImplTest() { From f56cb963dd64b58974247beddf9204721c12f402 Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 08:54:14 -0400 Subject: [PATCH 5/6] remove debug print statement. Signed-off-by: Joshua Marantz --- test/test_common/simulated_time_system.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/test_common/simulated_time_system.cc b/test/test_common/simulated_time_system.cc index 3f10036f31834..99df74a1cd7a2 100644 --- a/test/test_common/simulated_time_system.cc +++ b/test/test_common/simulated_time_system.cc @@ -140,7 +140,6 @@ SystemTime SimulatedTimeSystem::systemTime() { MonotonicTime SimulatedTimeSystem::monotonicTime() { Thread::LockGuard lock(mutex_); - std::cerr << "monotonic_time_=" << monotonic_time_.time_since_epoch().count() << std::endl; return monotonic_time_; } From 0655381f54af2f50b84314d27328ded6adcce67f Mon Sep 17 00:00:00 2001 From: Joshua Marantz Date: Wed, 3 Oct 2018 09:48:23 -0400 Subject: [PATCH 6/6] remove speculative generality for abstracting ConnectionImplTest::timeSystem(). That was added to allow FlushWriteAndDelayCloseTimerTriggerTest to use a MockTimeSystem. If that test is revived, we may need to bring back the virtual timeSystem() hook. Signed-off-by: Joshua Marantz --- test/common/network/connection_impl_test.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 04ed5091986da..db7a63c676246 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -89,7 +89,7 @@ class ConnectionImplTest : public testing::TestWithParam { public: void setUpBasicConnection() { if (dispatcher_.get() == nullptr) { - dispatcher_.reset(new Event::DispatcherImpl(timeSystem())); + dispatcher_.reset(new Event::DispatcherImpl(time_system_)); } listener_ = dispatcher_->createListener(socket_, listener_callbacks_, true, false); @@ -152,7 +152,7 @@ class ConnectionImplTest : public testing::TestWithParam { MockBufferFactory* factory = new StrictMock; dispatcher_.reset( - new Event::DispatcherImpl(timeSystem(), Buffer::WatermarkFactoryPtr{factory})); + new Event::DispatcherImpl(time_system_, Buffer::WatermarkFactoryPtr{factory})); // The first call to create a client session will get a MockBuffer. // Other calls for server sessions will by default get a normal OwnedImpl. EXPECT_CALL(*factory, create_(_, _)) @@ -168,8 +168,6 @@ class ConnectionImplTest : public testing::TestWithParam { })); } - virtual Event::TimeSystem& timeSystem() { return time_system_; } - protected: Event::SimulatedTimeSystem time_system_; Event::DispatcherPtr dispatcher_;