Fix a data race in the thread unit tests.

webrtc/base/thread_unittest.cc

Index: webrtc/base/thread_unittest.cc
diff --git a/webrtc/base/thread_unittest.cc b/webrtc/base/thread_unittest.cc
index e50e45cd8ab9ef87ff02b6fb04dffaa72cece51e..0ee0b3c99acea44342ea3f83e0a323f6d5f4c994 100644
--- a/webrtc/base/thread_unittest.cc
+++ b/webrtc/base/thread_unittest.cc
@@ -137,16 +137,51 @@ class SignalWhenDestroyedThread : public Thread {
   Event* event_;
 };
 
+// A bool wrapped in a mutex, do avoid data races. Using a volatile
+// bool should be sufficient for correct code ("eventual consistency"
+// between caches is sufficient), but we can't tell the compiler about
+// that, and then tsan complains about a data race.
+
+// See also discussion at
+// http://stackoverflow.com/questions/7223164/is-mutex-needed-to-synchronize-a-simple-flag-between-pthreads
+
+// Using std::atomic<bool> or std::atomic_flag in C++11 is probably
+// the right thing to do, but those features are not yet allowed.
+// Since the use isn't performance critical, use a plain mutex for the
+// time being.
+
+class AtomicFlag
+{

[magjed_webrtc, 2015/11/11 15:50:44]

You are not following the C++ style guide. The WebRTC style guide is essentially
the same as the Google style guide:
https://google.github.io/styleguide/cppguide.html
I strongly recommend using 'git cl format' to make sure formatting follows the
style guide.

[nisse-webrtc, 2015/11/12 08:39:05]

Done. I've also added a google style in my emacs cc-mode config.

[magjed_webrtc, 2015/11/12 09:20:51]

You can also take a look at go/emacs.

+public:
+  AtomicFlag(bool value) : flag(value) {}

[magjed_webrtc, 2015/11/11 15:50:44]

I'm not sure you are allowed to use implicit conversions like this. tommi wdyt?

[nisse-webrtc, 2015/11/12 08:39:05]

I can make it explicit. Only reason for implicit was that I tried to minimize
the changes where the flag is declared and initialized, from "bool called =
false;" to "AtomicFlag called = false;". With explicit, I have to change the
structure to "called(false)", which isn't a big deal, I guess.

I'm also considering adding a default initial value of false, does that make
sense? Then the declaration + initializer would be just "AtomicFlag called;".

[tommi, 2015/11/12 08:49:52]

Adding a default ctor that initializes to false makes sense to me.

[magjed_webrtc, 2015/11/12 09:20:51]

FYI - Here is a link to the style guide:
"Do not define implicit conversions. Use the explicit keyword for conversion
operators and single-argument constructors."
https://google.github.io/styleguide/cppguide.html#Implicit_Conversions
tommi seems fine with this and tommi > style guide.

[nisse-webrtc, 2015/11/12 09:58:23]

I changed the constructor to explicit (and with a default false).
Should I make the conversion to bool explicit too? Then I'd have to add some
conversion in the invocation of the EXPECT_TRUE_WAIT macro, which isn't too bad.

[nisse-webrtc, 2015/11/12 09:58:23]

Done.

[tommi, 2015/11/12 10:16:31]

hah!  Yeah, well, maybe the class could be called AtomicBool to make it clearer
that it should be considered the equivalent to the bool type.  By having a
default ctor though, do you consider that an implicit conversion to bool? (maybe
I'm missing the point)

[magjed_webrtc, 2015/11/12 11:04:38]

Yes, you should probably make the conversion to bool explicit if you want to
follow the style guide, and maybe also the copy assignment since you make a type
conversion there.
"Type conversion operators, and constructors that are callable with a single
argument, must be marked explicit in the class definition. As an exception, copy
and move constructors should not be explicit, since they do not perform type
conversion. Implicit conversions can sometimes be necessary and appropriate for
types that are designed to transparently wrap other types. In that case, contact
your project leads to request a waiver of this rule."
Tommi is the project lead, so it's up to him :)

Default ctor is fine. Another FYI is that default arguments is only allowed in
limited situations, but ctors is one of the exceptions:
https://google.github.io/styleguide/cppguide.html#Default_Arguments

+  bool operator= (bool value) {

[nisse-webrtc, 2015/11/12 08:39:05]

What's the proper return type for this assignment operator? Is it better to
return a reference to *this?

[tommi, 2015/11/12 08:49:52]

yes, return a reference to *this.

[magjed_webrtc, 2015/11/12 09:20:51]

The typical copy assignment operator would take the same type as argument, i.e.
AtomicFlag& operator= (const AtomicFlag& value)

[nisse-webrtc, 2015/11/12 09:58:23]

Done.

+    cs.Enter();

[magjed_webrtc, 2015/11/11 15:50:44]

Instead of manual Enter() and Leave(), you can use:
rtc::CritScope scoped_lock(&cs);

[nisse-webrtc, 2015/11/12 08:39:05]

Done.

+    flag = value;
+    cs.Leave();
+    return value;
+  }
+  operator bool() const {
+    bool value;
+    cs.Enter();
+    value = flag;
+    cs.Leave();
+    return value;
+  }
+private:
+  mutable CriticalSection cs;
+  bool flag;

[tommi, 2015/11/11 16:00:03]

fyi - pbos is working on a change that could be helpful:
https://codereview.webrtc.org/1420043008/

[nisse-webrtc, 2015/11/12 08:39:05]

I'm adding a reference to rtc::AtomicInt in the comment.

+};
+
 // Function objects to test Thread::Invoke.
 struct FunctorA {
   int operator()() { return 42; }
 };
 class FunctorB {
  public:
-  explicit FunctorB(bool* flag) : flag_(flag) {}
+  explicit FunctorB(AtomicFlag* flag) : flag_(flag) {}
   void operator()() { if (flag_) *flag_ = true; }
  private:
-  bool* flag_;
+  AtomicFlag* flag_;
 };
 struct FunctorC {
   int operator()() {
@@ -266,7 +301,7 @@ TEST(ThreadTest, Invoke) {
   thread.Start();
   // Try calling functors.
   EXPECT_EQ(42, thread.Invoke<int>(FunctorA()));
-  bool called = false;
+  AtomicFlag called = false;
   FunctorB f2(&called);
   thread.Invoke<void>(f2);
   EXPECT_TRUE(called);
@@ -408,7 +443,7 @@ TEST_F(AsyncInvokeTest, FireAndForget) {
   Thread thread;
   thread.Start();
   // Try calling functor.
-  bool called = false;
+  AtomicFlag called = false;
   invoker.AsyncInvoke<void>(&thread, FunctorB(&called));
   EXPECT_TRUE_WAIT(called, kWaitTimeout);
 }
@@ -478,8 +513,8 @@ TEST_F(AsyncInvokeTest, KillInvokerBeforeExecute) {
 
 TEST_F(AsyncInvokeTest, Flush) {
   AsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicFlag flag1 = false;
+  AtomicFlag flag2 = false;
   // Queue two async calls to the current thread.
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag1));
@@ -496,8 +531,8 @@ TEST_F(AsyncInvokeTest, Flush) {
 
 TEST_F(AsyncInvokeTest, FlushWithIds) {
   AsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicFlag flag1 = false;
+  AtomicFlag flag2 = false;
   // Queue two async calls to the current thread, one with a message id.
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag1),
@@ -564,7 +599,7 @@ TEST_F(GuardedAsyncInvokeTest, KillThreadFireAndForget) {
   // Kill |thread|.
   thread = nullptr;
   // Try calling functor.
-  bool called = false;
+  AtomicFlag called = false;
   EXPECT_FALSE(invoker->AsyncInvoke<void>(FunctorB(&called)));
   // With thread gone, nothing should happen.
   WAIT(called, kWaitTimeout);
@@ -595,7 +630,7 @@ TEST_F(GuardedAsyncInvokeTest, KillThreadWithCallback) {
 TEST_F(GuardedAsyncInvokeTest, FireAndForget) {
   GuardedAsyncInvoker invoker;
   // Try calling functor.
-  bool called = false;
+  AtomicFlag called = false;
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&called)));
   EXPECT_TRUE_WAIT(called, kWaitTimeout);
 }
@@ -660,8 +695,8 @@ TEST_F(GuardedAsyncInvokeTest, KillInvokerBeforeExecute) {
 
 TEST_F(GuardedAsyncInvokeTest, Flush) {
   GuardedAsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicFlag flag1 = false;
+  AtomicFlag flag2 = false;
   // Queue two async calls to the current thread.
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1)));
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));
@@ -676,8 +711,8 @@ TEST_F(GuardedAsyncInvokeTest, Flush) {
 
 TEST_F(GuardedAsyncInvokeTest, FlushWithIds) {
   GuardedAsyncInvoker invoker;
-  bool flag1 = false;
-  bool flag2 = false;
+  AtomicFlag flag1 = false;
+  AtomicFlag flag2 = false;
   // Queue two async calls to the current thread, one with a message id.
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag1), 5));
   EXPECT_TRUE(invoker.AsyncInvoke<void>(FunctorB(&flag2)));