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}
diff --git a/webrtc/base/base_tests.gyp b/webrtc/base/base_tests.gyp
index 003a1db..2ee115b 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 bcd720f..e5aa3bc 100644
--- a/webrtc/base/fakeclock.cc
+++ b/webrtc/base/fakeclock.cc
@@ -27,24 +27,14 @@
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 4aecc54..2b3afdd 100644
--- a/webrtc/base/fakeclock.h
+++ b/webrtc/base/fakeclock.h
@@ -19,8 +19,6 @@
// 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 @@
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 5ba0bd9..e705322 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 4c2331b..da50e23 100644
--- a/webrtc/base/messagequeue.cc
+++ b/webrtc/base/messagequeue.cc
@@ -9,14 +9,17 @@
*/
#include <algorithm>
-#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 @@
return instance_ != NULL;
}
-MessageQueueManager::MessageQueueManager() {}
+MessageQueueManager::MessageQueueManager() {
+}
MessageQueueManager::~MessageQueueManager() {
}
@@ -104,36 +108,26 @@
(*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<void, decltype(functor)> handler(functor);
- {
- CritScope cs(&crit_);
- queues_not_done = static_cast<int>(message_queues_.size());
- for (MessageQueue* queue : message_queues_) {
- queue->PostDelayed(0, &handler);
- }
+ 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);
}
- // 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);
- }
+}
+
+void MessageQueueManager::OnMessage(Message* pmsg) {
+ RTC_DCHECK(pmsg->message_id == MSG_WAKE_MESSAGE_QUEUE);
}
//------------------------------------------------------------------
diff --git a/webrtc/base/messagequeue.h b/webrtc/base/messagequeue.h
index bf11037..3a5226c 100644
--- a/webrtc/base/messagequeue.h
+++ b/webrtc/base/messagequeue.h
@@ -37,7 +37,7 @@
// 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 @@
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 0029acf..bbae91d 100644
--- a/webrtc/base/signalthread_unittest.cc
+++ b/webrtc/base/signalthread_unittest.cc
@@ -140,9 +140,6 @@
// 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<OwnerThread> owner(new OwnerThread(this));
main_thread_ = owner.get();
diff --git a/webrtc/base/timeutils.cc b/webrtc/base/timeutils.cc
index 3c89d80..ecd0911 100644
--- a/webrtc/base/timeutils.cc
+++ b/webrtc/base/timeutils.cc
@@ -32,10 +32,8 @@
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 113793a..78ebace 100644
--- a/webrtc/base/timeutils.h
+++ b/webrtc/base/timeutils.h
@@ -39,10 +39,8 @@
// 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 @@
// 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 d1cfe23..f183684 100644
--- a/webrtc/base/timeutils_unittest.cc
+++ b/webrtc/base/timeutils_unittest.cc
@@ -301,7 +301,7 @@
// 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 @@
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 @@
int64_t real_start_time_ms = TimeMillis();
FakeClock clock;
- SetClockForTesting(&clock);
+ SetClock(&clock);
Thread worker;
worker.Start();
@@ -359,25 +359,24 @@
message_handler_dispatched.Set();
};
FunctorMessageHandler<void, decltype(functor)> 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