From 22e0814d51ba730cb8a8eb420b1041e8e6e6ffd8 Mon Sep 17 00:00:00 2001 From: deadbeef Date: Mon, 12 Jun 2017 14:30:28 -0700 Subject: [PATCH] Update VirtualSocketServerTest to use a fake clock. Since this is a test for a fake network, it's only natural that it uses a fake clock as well. This makes the tests much faster, less flaky, and lets them be moved out of "webrtc_nonparallel_tests", since they no longer have a dependency on any "real" thing (sockets, or time) and can be run in parallel as easily as any other tests. As part of this CL, added the fake clock as an argument to VirtualSocketServer's and TestClient's constructors, since these classes have methods that wait synchronously for something to occur, and if the test is using a fake clock, they need to advance it in order to make progress. Lastly, added a DCHECK in Thread::ProcessMessages. If called with a nonzero time while a fake clock is used, it will get stuck in an infinite loop; a DCHECK is easier to notice than an infinite loop. BUG=webrtc:7727, webrtc:2409 Review-Url: https://codereview.webrtc.org/2927413002 Cr-Commit-Position: refs/heads/master@{#18544} --- webrtc/base/BUILD.gn | 7 ++-- webrtc/base/testclient.cc | 24 +++++++++-- webrtc/base/testclient.h | 7 ++++ webrtc/base/thread.cc | 5 +++ webrtc/base/timeutils.cc | 4 ++ webrtc/base/timeutils.h | 3 ++ webrtc/base/virtualsocket_unittest.cc | 57 +++++++++++++++------------ webrtc/base/virtualsocketserver.cc | 22 ++++++++--- webrtc/base/virtualsocketserver.h | 9 +++++ 9 files changed, 101 insertions(+), 37 deletions(-) diff --git a/webrtc/base/BUILD.gn b/webrtc/base/BUILD.gn index 2433371775..2c85c64b5f 100644 --- a/webrtc/base/BUILD.gn +++ b/webrtc/base/BUILD.gn @@ -532,9 +532,6 @@ rtc_static_library("rtc_base") { "optionsfile.h", "rollingaccumulator.h", "sslroots.h", - "testbase64.h", - "testclient.cc", - "testclient.h", "transformadapter.cc", "transformadapter.h", "window.h", @@ -707,6 +704,8 @@ rtc_source_set("rtc_base_tests_utils") { "sigslottester.h", "sigslottester.h.pump", "testbase64.h", + "testclient.cc", + "testclient.h", "testechoserver.h", "testutils.h", "timedelta.h", @@ -774,7 +773,6 @@ if (rtc_include_tests) { "socket_unittest.cc", "socket_unittest.h", "socketaddress_unittest.cc", - "virtualsocket_unittest.cc", ] deps = [ ":rtc_base", @@ -843,6 +841,7 @@ if (rtc_include_tests) { "thread_checker_unittest.cc", "timestampaligner_unittest.cc", "timeutils_unittest.cc", + "virtualsocket_unittest.cc", ] deps = [ ":rtc_base", diff --git a/webrtc/base/testclient.cc b/webrtc/base/testclient.cc index 7ec70f40d1..b6fd692350 100644 --- a/webrtc/base/testclient.cc +++ b/webrtc/base/testclient.cc @@ -9,6 +9,8 @@ */ #include "webrtc/base/testclient.h" + +#include "webrtc/base/gunit.h" #include "webrtc/base/ptr_util.h" #include "webrtc/base/thread.h" #include "webrtc/base/timeutils.h" @@ -20,7 +22,13 @@ namespace rtc { // NextPacket. TestClient::TestClient(std::unique_ptr socket) - : socket_(std::move(socket)), prev_packet_timestamp_(-1) { + : TestClient(std::move(socket), nullptr) {} + +TestClient::TestClient(std::unique_ptr socket, + FakeClock* fake_clock) + : fake_clock_(fake_clock), + socket_(std::move(socket)), + prev_packet_timestamp_(-1) { socket_->SignalReadPacket.connect(this, &TestClient::OnPacket); socket_->SignalReadyToSend.connect(this, &TestClient::OnReadyToSend); } @@ -31,7 +39,7 @@ bool TestClient::CheckConnState(AsyncPacketSocket::State state) { // Wait for our timeout value until the socket reaches the desired state. int64_t end = TimeAfter(kTimeoutMs); while (socket_->GetState() != state && TimeUntil(end) > 0) { - Thread::Current()->ProcessMessages(1); + AdvanceTime(1); } return (socket_->GetState() == state); } @@ -67,7 +75,7 @@ std::unique_ptr TestClient::NextPacket(int timeout_ms) { break; } } - Thread::Current()->ProcessMessages(1); + AdvanceTime(1); } // Return the first packet placed in the queue. @@ -108,6 +116,16 @@ bool TestClient::CheckTimestamp(int64_t packet_timestamp) { return res; } +void TestClient::AdvanceTime(int ms) { + // If the test is using a fake clock, we must advance the fake clock to + // advance time. Otherwise, ProcessMessages will work. + if (fake_clock_) { + SIMULATED_WAIT(false, ms, *fake_clock_); + } else { + Thread::Current()->ProcessMessages(1); + } +} + bool TestClient::CheckNoPacket() { return NextPacket(kNoPacketTimeoutMs) == nullptr; } diff --git a/webrtc/base/testclient.h b/webrtc/base/testclient.h index b4703828eb..bd78d83b35 100644 --- a/webrtc/base/testclient.h +++ b/webrtc/base/testclient.h @@ -16,6 +16,7 @@ #include "webrtc/base/asyncudpsocket.h" #include "webrtc/base/constructormagic.h" #include "webrtc/base/criticalsection.h" +#include "webrtc/base/fakeclock.h" namespace rtc { @@ -44,6 +45,10 @@ class TestClient : public sigslot::has_slots<> { // Creates a client that will send and receive with the given socket and // will post itself messages with the given thread. explicit TestClient(std::unique_ptr socket); + // Create a test client that will use a fake clock. NextPacket needs to wait + // for a packet to be received, and thus it needs to advance the fake clock + // if the test is using one, rather than just sleeping. + TestClient(std::unique_ptr socket, FakeClock* fake_clock); ~TestClient() override; SocketAddress address() const { return socket_->GetLocalAddress(); } @@ -93,7 +98,9 @@ class TestClient : public sigslot::has_slots<> { const PacketTime& packet_time); void OnReadyToSend(AsyncPacketSocket* socket); bool CheckTimestamp(int64_t packet_timestamp); + void AdvanceTime(int ms); + FakeClock* fake_clock_ = nullptr; CriticalSection crit_; std::unique_ptr socket_; std::vector> packets_; diff --git a/webrtc/base/thread.cc b/webrtc/base/thread.cc index 1be5196c7d..9174cd1bd6 100644 --- a/webrtc/base/thread.cc +++ b/webrtc/base/thread.cc @@ -471,6 +471,11 @@ void Thread::Clear(MessageHandler* phandler, // Note that these methods have a separate implementation for mac and ios // defined in webrtc/base/thread_darwin.mm. bool Thread::ProcessMessages(int cmsLoop) { + // Using ProcessMessages with a custom clock for testing and a time greater + // than 0 doesn't work, since it's not guaranteed to advance the custom + // clock's time, and may get stuck in an infinite loop. + RTC_DCHECK(GetClockForTesting() == nullptr || cmsLoop == 0 || + cmsLoop == kForever); int64_t msEnd = (kForever == cmsLoop) ? 0 : TimeAfter(cmsLoop); int cmsNext = cmsLoop; diff --git a/webrtc/base/timeutils.cc b/webrtc/base/timeutils.cc index 7fcc2f4149..ee1415f4d2 100644 --- a/webrtc/base/timeutils.cc +++ b/webrtc/base/timeutils.cc @@ -39,6 +39,10 @@ ClockInterface* SetClockForTesting(ClockInterface* clock) { return prev; } +ClockInterface* GetClockForTesting() { + return g_clock; +} + int64_t SystemTimeNanos() { int64_t ticks; #if defined(WEBRTC_MAC) diff --git a/webrtc/base/timeutils.h b/webrtc/base/timeutils.h index be9edabf2d..735af4a9f4 100644 --- a/webrtc/base/timeutils.h +++ b/webrtc/base/timeutils.h @@ -53,6 +53,9 @@ class ClockInterface { // that uses it, eliminating the need for a global variable and this function. ClockInterface* SetClockForTesting(ClockInterface* clock); +// Returns previously set clock, or nullptr if no custom clock is being used. +ClockInterface* GetClockForTesting(); + // Returns the actual system time, even if a clock is set for testing. // Useful for timeouts while using a test clock, or for logging. int64_t SystemTimeNanos(); diff --git a/webrtc/base/virtualsocket_unittest.cc b/webrtc/base/virtualsocket_unittest.cc index 5f62fd1ae2..34ee036c8a 100644 --- a/webrtc/base/virtualsocket_unittest.cc +++ b/webrtc/base/virtualsocket_unittest.cc @@ -17,6 +17,7 @@ #include #include "webrtc/base/arraysize.h" +#include "webrtc/base/fakeclock.h" #include "webrtc/base/gunit.h" #include "webrtc/base/logging.h" #include "webrtc/base/ptr_util.h" @@ -143,10 +144,12 @@ struct Receiver : public MessageHandler, public sigslot::has_slots<> { uint32_t samples; }; +// Note: This test uses a fake clock in addition to a virtual network. class VirtualSocketServerTest : public testing::Test { public: VirtualSocketServerTest() - : thread_(&ss_), + : ss_(&fake_clock_), + thread_(&ss_), kIPv4AnyAddress(IPAddress(INADDR_ANY), 0), kIPv6AnyAddress(IPAddress(in6addr_any), 0) {} @@ -181,7 +184,8 @@ class VirtualSocketServerTest : public testing::Test { socket->Bind(EmptySocketAddressWithFamily(default_route.family())); SocketAddress client1_any_addr = socket->GetLocalAddress(); EXPECT_TRUE(client1_any_addr.IsAnyIP()); - auto client1 = MakeUnique(MakeUnique(socket)); + auto client1 = MakeUnique(MakeUnique(socket), + &fake_clock_); // Create client2 bound to the default route. AsyncSocket* socket2 = @@ -189,7 +193,8 @@ class VirtualSocketServerTest : public testing::Test { socket2->Bind(SocketAddress(default_route, 0)); SocketAddress client2_addr = socket2->GetLocalAddress(); EXPECT_FALSE(client2_addr.IsAnyIP()); - auto client2 = MakeUnique(MakeUnique(socket2)); + auto client2 = MakeUnique(MakeUnique(socket2), + &fake_clock_); // Client1 sends to client2, client2 should see the default route as // client1's address. @@ -212,10 +217,12 @@ class VirtualSocketServerTest : public testing::Test { // Make sure VSS didn't switch families on us. EXPECT_EQ(server_addr.family(), initial_addr.family()); - auto client1 = MakeUnique(MakeUnique(socket)); + auto client1 = MakeUnique(MakeUnique(socket), + &fake_clock_); AsyncSocket* socket2 = ss_.CreateAsyncSocket(initial_addr.family(), SOCK_DGRAM); - auto client2 = MakeUnique(MakeUnique(socket2)); + auto client2 = MakeUnique(MakeUnique(socket2), + &fake_clock_); SocketAddress client2_addr; EXPECT_EQ(3, client2->SendTo("foo", 3, server_addr)); @@ -229,7 +236,7 @@ class VirtualSocketServerTest : public testing::Test { SocketAddress empty = EmptySocketAddressWithFamily(initial_addr.family()); for (int i = 0; i < 10; i++) { client2 = MakeUnique( - WrapUnique(AsyncUDPSocket::Create(&ss_, empty))); + WrapUnique(AsyncUDPSocket::Create(&ss_, empty)), &fake_clock_); SocketAddress next_client2_addr; EXPECT_EQ(3, client2->SendTo("foo", 3, server_addr)); @@ -684,12 +691,15 @@ class VirtualSocketServerTest : public testing::Test { Sender sender(pthMain, send_socket, 80 * 1024); Receiver receiver(pthMain, recv_socket, bandwidth); - pthMain->ProcessMessages(5000); + // Allow the sender to run for 5 (simulated) seconds, then be stopped for 5 + // seconds. + SIMULATED_WAIT(false, 5000, fake_clock_); sender.done = true; - pthMain->ProcessMessages(5000); + SIMULATED_WAIT(false, 5000, fake_clock_); - ASSERT_TRUE(receiver.count >= 5 * 3 * bandwidth / 4); - ASSERT_TRUE(receiver.count <= 6 * bandwidth); // queue could drain for 1s + // Ensure the observed bandwidth fell within a reasonable margin of error. + EXPECT_TRUE(receiver.count >= 5 * 3 * bandwidth / 4); + EXPECT_TRUE(receiver.count <= 6 * bandwidth); // queue could drain for 1s ss_.set_bandwidth(0); } @@ -725,7 +735,9 @@ class VirtualSocketServerTest : public testing::Test { Sender sender(pthMain, send_socket, 100 * 2 * 1024); Receiver receiver(pthMain, recv_socket, 0); - pthMain->ProcessMessages(10000); + // Simulate 10 seconds of packets being sent, then check the observed delay + // distribution. + SIMULATED_WAIT(false, 10000, fake_clock_); sender.done = receiver.done = true; ss_.ProcessMessagesUntilIdle(); @@ -807,11 +819,13 @@ class VirtualSocketServerTest : public testing::Test { AsyncSocket* socket = ss_.CreateAsyncSocket(SOCK_DGRAM); socket->Bind(server_addr); SocketAddress bound_server_addr = socket->GetLocalAddress(); - auto client1 = MakeUnique(MakeUnique(socket)); + auto client1 = MakeUnique(MakeUnique(socket), + &fake_clock_); AsyncSocket* socket2 = ss_.CreateAsyncSocket(SOCK_DGRAM); socket2->Bind(client_addr); - auto client2 = MakeUnique(MakeUnique(socket2)); + auto client2 = MakeUnique(MakeUnique(socket2), + &fake_clock_); SocketAddress client2_addr; if (shouldSucceed) { @@ -828,6 +842,7 @@ class VirtualSocketServerTest : public testing::Test { } protected: + rtc::ScopedFakeClock fake_clock_; VirtualSocketServer ss_; AutoSocketServerThread thread_; const SocketAddress kIPv4AnyAddress; @@ -912,20 +927,11 @@ TEST_F(VirtualSocketServerTest, bandwidth_v6) { BandwidthTest(kIPv6AnyAddress); } -// Disabled on iOS simulator since it's a test that relies on being able to -// process packets fast enough in real time, which isn't the case in the -// simulator. -#if defined(TARGET_IPHONE_SIMULATOR) -#define MAYBE_delay_v4 DISABLED_delay_v4 -#else -#define MAYBE_delay_v4 delay_v4 -#endif -TEST_F(VirtualSocketServerTest, MAYBE_delay_v4) { +TEST_F(VirtualSocketServerTest, delay_v4) { DelayTest(kIPv4AnyAddress); } -// See: https://code.google.com/p/webrtc/issues/detail?id=2409 -TEST_F(VirtualSocketServerTest, DISABLED_delay_v6) { +TEST_F(VirtualSocketServerTest, delay_v6) { DelayTest(kIPv6AnyAddress); } @@ -1040,7 +1046,8 @@ TEST_F(VirtualSocketServerTest, SetSendingBlockedWithUdpSocket) { WrapUnique(ss_.CreateAsyncSocket(kIPv4AnyAddress.family(), SOCK_DGRAM)); socket1->Bind(kIPv4AnyAddress); socket2->Bind(kIPv4AnyAddress); - auto client1 = MakeUnique(MakeUnique(socket1)); + auto client1 = + MakeUnique(MakeUnique(socket1), &fake_clock_); ss_.SetSendingBlocked(true); EXPECT_EQ(-1, client1->SendTo("foo", 3, socket2->GetLocalAddress())); diff --git a/webrtc/base/virtualsocketserver.cc b/webrtc/base/virtualsocketserver.cc index cfb4eb294b..95feeae49a 100644 --- a/webrtc/base/virtualsocketserver.cc +++ b/webrtc/base/virtualsocketserver.cc @@ -19,6 +19,7 @@ #include #include "webrtc/base/checks.h" +#include "webrtc/base/gunit.h" #include "webrtc/base/logging.h" #include "webrtc/base/physicalsocketserver.h" #include "webrtc/base/socketaddresspair.h" @@ -528,8 +529,11 @@ void VirtualSocket::OnSocketServerReadyToSend() { } } -VirtualSocketServer::VirtualSocketServer() - : wakeup_(/*manual_reset=*/false, /*initially_signaled=*/false), +VirtualSocketServer::VirtualSocketServer() : VirtualSocketServer(nullptr) {} + +VirtualSocketServer::VirtualSocketServer(FakeClock* fake_clock) + : fake_clock_(fake_clock), + wakeup_(/*manual_reset=*/false, /*initially_signaled=*/false), msg_queue_(nullptr), stop_on_idle_(false), next_ipv4_(kInitialNextIPv4), @@ -642,9 +646,17 @@ bool VirtualSocketServer::ProcessMessagesUntilIdle() { RTC_DCHECK(msg_queue_ == Thread::Current()); stop_on_idle_ = true; while (!msg_queue_->empty()) { - Message msg; - if (msg_queue_->Get(&msg, Thread::kForever)) { - msg_queue_->Dispatch(&msg); + if (fake_clock_) { + // If using a fake clock, advance it in millisecond increments until the + // queue is empty (the SIMULATED_WAIT macro ensures the queue processes + // all possible messages each time it's called). + SIMULATED_WAIT(false, 1, *fake_clock_); + } else { + // Otherwise, run a normal message loop. + Message msg; + if (msg_queue_->Get(&msg, Thread::kForever)) { + msg_queue_->Dispatch(&msg); + } } } stop_on_idle_ = false; diff --git a/webrtc/base/virtualsocketserver.h b/webrtc/base/virtualsocketserver.h index 56535f1783..0ab2af29fb 100644 --- a/webrtc/base/virtualsocketserver.h +++ b/webrtc/base/virtualsocketserver.h @@ -17,6 +17,7 @@ #include "webrtc/base/checks.h" #include "webrtc/base/constructormagic.h" #include "webrtc/base/event.h" +#include "webrtc/base/fakeclock.h" #include "webrtc/base/messagequeue.h" #include "webrtc/base/socketserver.h" @@ -33,6 +34,10 @@ class SocketAddressPair; class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> { public: VirtualSocketServer(); + // This constructor needs to be used if the test uses a fake clock and + // ProcessMessagesUntilIdle, since ProcessMessagesUntilIdle needs a way of + // advancing time. + explicit VirtualSocketServer(FakeClock* fake_clock); ~VirtualSocketServer() override; // The default route indicates which local address to use when a socket is @@ -242,6 +247,10 @@ class VirtualSocketServer : public SocketServer, public sigslot::has_slots<> { typedef std::map AddressMap; typedef std::map ConnectionMap; + // May be null if the test doesn't use a fake clock, or it does but doesn't + // use ProcessMessagesUntilIdle. + FakeClock* fake_clock_ = nullptr; + // Used to implement Wait/WakeUp. Event wakeup_; MessageQueue* msg_queue_;