From f83a94a41e82c27b01166d590a422ca63a0a7bca Mon Sep 17 00:00:00 2001 From: deadbeef Date: Fri, 3 Jun 2016 16:05:26 -0700 Subject: [PATCH] Revert of Improving the fake clock and using it to fix a flaky STUN timeout test. (patchset #10 id:180001 of https://codereview.webrtc.org/2024813004/ ) Reason for revert: There seems to be a TSan warning that wasn't caught by the trybot: https://build.chromium.org/p/client.webrtc/builders/Linux%20Tsan%20v2/builds/6732/steps/peerconnection_unittests/logs/stdio Apparently a thread is still alive even after destroying WebRTCSession. Need to think of a way to fix this, without adding a critical section around g_clock (since that would hurt performance). Original issue's description: > Improving the fake clock and using it to fix a flaky STUN timeout test. > > When the fake clock's time is advanced, it now ensures all pending > queued messages have been dispatched. This allows us to write a > "SIMULATED_WAIT" macro that ticks the simulated clock by milliseconds up > until the target time. > > Useful in this case, where we know the STUN timeout should take a total > of 9500ms, but it would be overly complex to write test code that waits > for each individual timeout, ensures a STUN packet has been > retransmited, etc. > > (The test described above *should* be written, but it belongs in > p2ptransportchannel_unittest.cc, not webrtcsession_unittest.cc). > > Committed: https://crrev.com/ffbe0e17e2c9b7fe101023acf40574dc0c95631a > Cr-Commit-Position: refs/heads/master@{#13043} TBR=pthatcher@webrtc.org,tommi@webrtc.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true Review-Url: https://codereview.webrtc.org/2038213002 Cr-Commit-Position: refs/heads/master@{#13045} --- webrtc/api/webrtcsession_unittest.cc | 17 +++++---- webrtc/base/base_tests.gyp | 3 -- webrtc/base/fakeclock.cc | 20 +++-------- webrtc/base/fakeclock.h | 13 ------- webrtc/base/gunit.h | 54 +++------------------------- webrtc/base/messagequeue.cc | 42 ++++++++++------------ webrtc/base/messagequeue.h | 13 +++---- webrtc/base/signalthread_unittest.cc | 3 -- webrtc/base/timeutils.cc | 4 +-- webrtc/base/timeutils.h | 8 ++--- webrtc/base/timeutils_unittest.cc | 25 +++++++------ webrtc/webrtc_tests.gypi | 3 ++ 12 files changed, 62 insertions(+), 143 deletions(-) diff --git a/webrtc/api/webrtcsession_unittest.cc b/webrtc/api/webrtcsession_unittest.cc index 01db8ffca8..088694cb16 100644 --- a/webrtc/api/webrtcsession_unittest.cc +++ b/webrtc/api/webrtcsession_unittest.cc @@ -109,8 +109,6 @@ static const int kMediaContentIndex1 = 1; static const char kMediaContentName1[] = "video"; static const int kIceCandidatesTimeout = 10000; -// STUN timeout with all retransmissions is a total of 9500ms. -static const int kStunTimeout = 9500; static const char kFakeDtlsFingerprint[] = "BB:CD:72:F7:2F:D0:BA:43:F3:68:B1:0C:23:72:B6:4A:" @@ -1506,9 +1504,13 @@ TEST_F(WebRtcSessionTest, TestMultihomeCandidates) { EXPECT_EQ(8u, observer_.mline_1_candidates_.size()); } -TEST_F(WebRtcSessionTest, TestStunError) { - rtc::ScopedFakeClock clock; - +// Crashes on Win only. See webrtc:5411. +#if defined(WEBRTC_WIN) +#define MAYBE_TestStunError DISABLED_TestStunError +#else +#define MAYBE_TestStunError TestStunError +#endif +TEST_F(WebRtcSessionTest, MAYBE_TestStunError) { AddInterface(rtc::SocketAddress(kClientAddrHost1, kClientAddrPort)); AddInterface(rtc::SocketAddress(kClientAddrHost2, kClientAddrPort)); fss_->AddRule(false, @@ -1519,12 +1521,9 @@ TEST_F(WebRtcSessionTest, TestStunError) { SendAudioVideoStream1(); InitiateCall(); // Since kClientAddrHost1 is blocked, not expecting stun candidates for it. - EXPECT_TRUE_SIMULATED_WAIT(observer_.oncandidatesready_, kStunTimeout, clock); + EXPECT_TRUE_WAIT(observer_.oncandidatesready_, kIceCandidatesTimeout); EXPECT_EQ(6u, observer_.mline_0_candidates_.size()); EXPECT_EQ(6u, observer_.mline_1_candidates_.size()); - // Reset session before scoped fake clock goes out of scope to avoid TSan - // warning. - session_.reset(nullptr); } TEST_F(WebRtcSessionTest, SetSdpFailedOnInvalidSdp) { diff --git a/webrtc/base/base_tests.gyp b/webrtc/base/base_tests.gyp index 003a1dbf87..2ee115b267 100644 --- a/webrtc/base/base_tests.gyp +++ b/webrtc/base/base_tests.gyp @@ -15,8 +15,6 @@ 'unittest_main.cc', # Also use this as a convenient dumping ground for misc files that are # included by multiple targets below. - 'fakeclock.cc', - 'fakeclock.h', 'fakenetwork.h', 'fakesslidentity.h', 'faketaskrunner.h', @@ -24,7 +22,6 @@ 'testbase64.h', 'testechoserver.h', 'testutils.h', - 'timedelta.h', ], 'defines': [ 'GTEST_RELATIVE_PATH', diff --git a/webrtc/base/fakeclock.cc b/webrtc/base/fakeclock.cc index bcd720ff74..e5aa3bc0a4 100644 --- a/webrtc/base/fakeclock.cc +++ b/webrtc/base/fakeclock.cc @@ -27,24 +27,14 @@ void FakeClock::SetTimeNanos(uint64_t nanos) { time_ = nanos; } // If message queues are waiting in a socket select() with a timeout provided - // by the OS, they should wake up and dispatch all messages that are ready. - MessageQueueManager::ProcessAllMessageQueues(); + // by the OS, they should wake up to check if there are any messages ready to + // be dispatched based on the fake time. + MessageQueueManager::WakeAllMessageQueues(); } void FakeClock::AdvanceTime(TimeDelta delta) { - { - CritScope cs(&lock_); - time_ += delta.ToNanoseconds(); - } - MessageQueueManager::ProcessAllMessageQueues(); -} - -ScopedFakeClock::ScopedFakeClock() { - prev_clock_ = SetClockForTesting(this); -} - -ScopedFakeClock::~ScopedFakeClock() { - SetClockForTesting(prev_clock_); + CritScope cs(&lock_); + SetTimeNanos(time_ + delta.ToNanoseconds()); } } // namespace rtc diff --git a/webrtc/base/fakeclock.h b/webrtc/base/fakeclock.h index 4aecc54cc4..2b3afdde05 100644 --- a/webrtc/base/fakeclock.h +++ b/webrtc/base/fakeclock.h @@ -19,8 +19,6 @@ namespace rtc { // Fake clock for use with unit tests, which does not tick on its own. // Starts at time 0. -// -// TODO(deadbeef): Unify with webrtc::SimulatedClock. class FakeClock : public ClockInterface { public: ~FakeClock() override {} @@ -40,17 +38,6 @@ class FakeClock : public ClockInterface { uint64_t time_ GUARDED_BY(lock_) = 0u; }; -// Helper class that sets itself as the global clock in its constructor and -// unsets it in its destructor. -class ScopedFakeClock : public FakeClock { - public: - ScopedFakeClock(); - ~ScopedFakeClock() override; - - private: - ClockInterface* prev_clock_; -}; - } // namespace rtc #endif // WEBRTC_BASE_FAKECLOCK_H_ diff --git a/webrtc/base/gunit.h b/webrtc/base/gunit.h index 5ba0bd916e..e705322e6f 100644 --- a/webrtc/base/gunit.h +++ b/webrtc/base/gunit.h @@ -11,7 +11,6 @@ #ifndef WEBRTC_BASE_GUNIT_H_ #define WEBRTC_BASE_GUNIT_H_ -#include "webrtc/base/fakeclock.h" #include "webrtc/base/logging.h" #include "webrtc/base/thread.h" #if defined(GTEST_RELATIVE_PATH) @@ -21,12 +20,10 @@ #endif // Wait until "ex" is true, or "timeout" expires. -#define WAIT(ex, timeout) \ - for (int64_t start = rtc::TimeMillis(); \ - !(ex) && rtc::TimeMillis() < start + timeout;) { \ - rtc::Thread::Current()->ProcessMessages(0); \ - rtc::Thread::Current()->SleepMs(1); \ - } +#define WAIT(ex, timeout) \ + for (int64_t start = rtc::TimeMillis(); \ + !(ex) && rtc::TimeMillis() < start + timeout;) \ + rtc::Thread::Current()->ProcessMessages(1); // This returns the result of the test in res, so that we don't re-evaluate // the expression in the XXXX_WAIT macros below, since that causes problems @@ -36,8 +33,7 @@ int64_t start = rtc::TimeMillis(); \ res = (ex); \ while (!res && rtc::TimeMillis() < start + timeout) { \ - rtc::Thread::Current()->ProcessMessages(0); \ - rtc::Thread::Current()->SleepMs(1); \ + rtc::Thread::Current()->ProcessMessages(1); \ res = (ex); \ } \ } while (0) @@ -89,44 +85,4 @@ } \ } while (0) -// Wait until "ex" is true, or "timeout" expires, using fake clock where -// messages are processed every millisecond. -#define SIMULATED_WAIT(ex, timeout, clock) \ - for (int64_t start = rtc::TimeMillis(); \ - !(ex) && rtc::TimeMillis() < start + timeout;) { \ - clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1)); \ - } - -// This returns the result of the test in res, so that we don't re-evaluate -// the expression in the XXXX_WAIT macros below, since that causes problems -// when the expression is only true the first time you check it. -#define SIMULATED_WAIT_(ex, timeout, res, clock) \ - do { \ - int64_t start = rtc::TimeMillis(); \ - res = (ex); \ - while (!res && rtc::TimeMillis() < start + timeout) { \ - clock.AdvanceTime(rtc::TimeDelta::FromMilliseconds(1)); \ - res = (ex); \ - } \ - } while (0) - -// The typical EXPECT_XXXX, but done until true or a timeout with a fake clock. -#define EXPECT_TRUE_SIMULATED_WAIT(ex, timeout, clock) \ - do { \ - bool res; \ - SIMULATED_WAIT_(ex, timeout, res, clock); \ - if (!res) { \ - EXPECT_TRUE(ex); \ - } \ - } while (0) - -#define EXPECT_EQ_SIMULATED_WAIT(v1, v2, timeout, clock) \ - do { \ - bool res; \ - SIMULATED_WAIT_(v1 == v2, timeout, res, clock); \ - if (!res) { \ - EXPECT_EQ(v1, v2); \ - } \ - } while (0) - #endif // WEBRTC_BASE_GUNIT_H_ diff --git a/webrtc/base/messagequeue.cc b/webrtc/base/messagequeue.cc index 4c2331bfe6..da50e2304f 100644 --- a/webrtc/base/messagequeue.cc +++ b/webrtc/base/messagequeue.cc @@ -9,14 +9,17 @@ */ #include -#include "webrtc/base/atomicops.h" #include "webrtc/base/checks.h" #include "webrtc/base/common.h" #include "webrtc/base/logging.h" #include "webrtc/base/messagequeue.h" -#include "webrtc/base/thread.h" #include "webrtc/base/trace_event.h" +namespace { + +enum { MSG_WAKE_MESSAGE_QUEUE = 1 }; +} + namespace rtc { const int kMaxMsgLatency = 150; // 150 ms @@ -38,7 +41,8 @@ bool MessageQueueManager::IsInitialized() { return instance_ != NULL; } -MessageQueueManager::MessageQueueManager() {} +MessageQueueManager::MessageQueueManager() { +} MessageQueueManager::~MessageQueueManager() { } @@ -104,38 +108,28 @@ void MessageQueueManager::ClearInternal(MessageHandler *handler) { (*iter)->Clear(handler); } -void MessageQueueManager::ProcessAllMessageQueues() { +void MessageQueueManager::WakeAllMessageQueues() { if (!instance_) { return; } - return Instance()->ProcessAllMessageQueuesInternal(); + return Instance()->WakeAllMessageQueuesInternal(); } -void MessageQueueManager::ProcessAllMessageQueuesInternal() { +void MessageQueueManager::WakeAllMessageQueuesInternal() { #if CS_DEBUG_CHECKS // CurrentThreadIsOwner returns true by default. ASSERT(!crit_.CurrentThreadIsOwner()); // See note above. #endif - // Post a delayed message at the current time and wait for it to be dispatched - // on all queues, which will ensure that all messages that came before it were - // also dispatched. - volatile int queues_not_done; - auto functor = [&queues_not_done] { AtomicOps::Decrement(&queues_not_done); }; - FunctorMessageHandler handler(functor); - { - CritScope cs(&crit_); - queues_not_done = static_cast(message_queues_.size()); - for (MessageQueue* queue : message_queues_) { - queue->PostDelayed(0, &handler); - } - } - // Note: One of the message queues may have been on this thread, which is why - // we can't synchronously wait for queues_not_done to go to 0; we need to - // process messages as well. - while (AtomicOps::AcquireLoad(&queues_not_done) > 0) { - rtc::Thread::Current()->ProcessMessages(0); + CritScope cs(&crit_); + for (MessageQueue* queue : message_queues_) { + // Posting an arbitrary message will force the message queue to wake up. + queue->Post(this, MSG_WAKE_MESSAGE_QUEUE); } } +void MessageQueueManager::OnMessage(Message* pmsg) { + RTC_DCHECK(pmsg->message_id == MSG_WAKE_MESSAGE_QUEUE); +} + //------------------------------------------------------------------ // MessageQueue MessageQueue::MessageQueue(SocketServer* ss, bool init_queue) diff --git a/webrtc/base/messagequeue.h b/webrtc/base/messagequeue.h index bf110376b5..3a5226cd0a 100644 --- a/webrtc/base/messagequeue.h +++ b/webrtc/base/messagequeue.h @@ -37,7 +37,7 @@ class MessageQueue; // MessageQueueManager does cleanup of of message queues -class MessageQueueManager { +class MessageQueueManager : public MessageHandler { public: static void Add(MessageQueue *message_queue); static void Remove(MessageQueue *message_queue); @@ -50,20 +50,21 @@ class MessageQueueManager { static bool IsInitialized(); // Mainly for testing purposes, for use with a simulated clock. - // Ensures that all message queues have processed delayed messages - // up until the current point in time. - static void ProcessAllMessageQueues(); + // Posts a no-op event on all message queues so they will wake from the + // socket server select() and process messages again. + static void WakeAllMessageQueues(); private: static MessageQueueManager* Instance(); MessageQueueManager(); - ~MessageQueueManager(); + ~MessageQueueManager() override; void AddInternal(MessageQueue *message_queue); void RemoveInternal(MessageQueue *message_queue); void ClearInternal(MessageHandler *handler); - void ProcessAllMessageQueuesInternal(); + void WakeAllMessageQueuesInternal(); + void OnMessage(Message* pmsg) override; static MessageQueueManager* instance_; // This list contains all live MessageQueues. diff --git a/webrtc/base/signalthread_unittest.cc b/webrtc/base/signalthread_unittest.cc index 0029acf5d9..bbae91daf0 100644 --- a/webrtc/base/signalthread_unittest.cc +++ b/webrtc/base/signalthread_unittest.cc @@ -140,9 +140,6 @@ class OwnerThread : public Thread, public sigslot::has_slots<> { // signal thread is still working. This may happen // when shutting down the process. TEST_F(SignalThreadTest, OwnerThreadGoesAway) { - // We don't use |thread_| for this test, so destroy it. - thread_->Destroy(true); - { std::unique_ptr owner(new OwnerThread(this)); main_thread_ = owner.get(); diff --git a/webrtc/base/timeutils.cc b/webrtc/base/timeutils.cc index 3c89d808b3..ecd0911a51 100644 --- a/webrtc/base/timeutils.cc +++ b/webrtc/base/timeutils.cc @@ -32,10 +32,8 @@ namespace rtc { ClockInterface* g_clock = nullptr; -ClockInterface* SetClockForTesting(ClockInterface* clock) { - ClockInterface* prev = g_clock; +void SetClock(ClockInterface* clock) { g_clock = clock; - return prev; } uint64_t TimeNanos() { diff --git a/webrtc/base/timeutils.h b/webrtc/base/timeutils.h index 113793ade3..78ebacee38 100644 --- a/webrtc/base/timeutils.h +++ b/webrtc/base/timeutils.h @@ -39,10 +39,8 @@ class ClockInterface { // Sets the global source of time. This is useful mainly for unit tests. // -// Returns the previously set ClockInterface, or nullptr if none is set. -// -// Does not transfer ownership of the clock. SetClockForTesting(nullptr) -// should be called before the ClockInterface is deleted. +// Does not transfer ownership of the clock. +// SetClock(nullptr) should be called before the ClockInterface is deleted. // // This method is not thread-safe; it should only be used when no other thread // is running (for example, at the start/end of a unit test, or start/end of @@ -51,7 +49,7 @@ class ClockInterface { // TODO(deadbeef): Instead of having functions that access this global // ClockInterface, we may want to pass the ClockInterface into everything // that uses it, eliminating the need for a global variable and this function. -ClockInterface* SetClockForTesting(ClockInterface* clock); +void SetClock(ClockInterface* clock); // Returns the current time in milliseconds in 32 bits. uint32_t Time32(); diff --git a/webrtc/base/timeutils_unittest.cc b/webrtc/base/timeutils_unittest.cc index d1cfe23821..f183684e6c 100644 --- a/webrtc/base/timeutils_unittest.cc +++ b/webrtc/base/timeutils_unittest.cc @@ -301,7 +301,7 @@ TEST(TimeDelta, NumericOperators) { // fake clock when it's set. TEST(FakeClock, TimeFunctionsUseFakeClock) { FakeClock clock; - SetClockForTesting(&clock); + SetClock(&clock); clock.SetTimeNanos(987654321u); EXPECT_EQ(987u, Time32()); @@ -310,7 +310,7 @@ TEST(FakeClock, TimeFunctionsUseFakeClock) { EXPECT_EQ(987654321u, TimeNanos()); EXPECT_EQ(1000u, TimeAfter(13)); - SetClockForTesting(nullptr); + SetClock(nullptr); // After it's unset, we should get a normal time. EXPECT_NE(987, TimeMillis()); } @@ -348,7 +348,7 @@ TEST(FakeClock, SettingTimeWakesThreads) { int64_t real_start_time_ms = TimeMillis(); FakeClock clock; - SetClockForTesting(&clock); + SetClock(&clock); Thread worker; worker.Start(); @@ -359,25 +359,24 @@ TEST(FakeClock, SettingTimeWakesThreads) { message_handler_dispatched.Set(); }; FunctorMessageHandler handler(functor); - worker.PostDelayed(60000, &handler); + worker.PostDelayed(10000, &handler); // Wait for a bit for the worker thread to be started and enter its socket - // select(). Otherwise this test would be trivial since the worker thread - // would process the event as soon as it was started. + // select(). Thread::Current()->SleepMs(1000); // Advance the fake clock, expecting the worker thread to wake up - // and dispatch the message instantly. - clock.AdvanceTime(TimeDelta::FromSeconds(60u)); - EXPECT_TRUE(message_handler_dispatched.Wait(0)); + // and dispatch the message quickly. + clock.AdvanceTime(TimeDelta::FromSeconds(10u)); + message_handler_dispatched.Wait(Event::kForever); worker.Stop(); - SetClockForTesting(nullptr); + SetClock(nullptr); - // The message should have been dispatched long before the 60 seconds fully - // elapsed (just a sanity check). + // The message should have been dispatched long before the 10 seconds fully + // elapsed. int64_t real_end_time_ms = TimeMillis(); - EXPECT_LT(real_end_time_ms - real_start_time_ms, 10000); + EXPECT_LT(real_end_time_ms - real_start_time_ms, 2000); } } // namespace rtc diff --git a/webrtc/webrtc_tests.gypi b/webrtc/webrtc_tests.gypi index e9f24456ca..4177cfeb5e 100644 --- a/webrtc/webrtc_tests.gypi +++ b/webrtc/webrtc_tests.gypi @@ -39,6 +39,8 @@ 'base/event_tracer_unittest.cc', 'base/event_unittest.cc', 'base/exp_filter_unittest.cc', + 'base/fakeclock.cc', + 'base/fakeclock.h', 'base/filerotatingstream_unittest.cc', 'base/fileutils_unittest.cc', 'base/helpers_unittest.cc', @@ -88,6 +90,7 @@ 'base/testclient_unittest.cc', 'base/thread_checker_unittest.cc', 'base/thread_unittest.cc', + 'base/timedelta.h', 'base/timeutils_unittest.cc', 'base/urlencode_unittest.cc', 'base/versionparsing_unittest.cc',