Fix a data race in the thread unit tests.

webrtc/base/thread_unittest.cc

 /*
  *  Copyright 2004 The WebRTC Project Authors. All rights reserved.
  *
  *  Use of this source code is governed by a BSD-style license
  *  that can be found in the LICENSE file in the root of the source
  *  tree. An additional intellectual property rights grant can be found
  *  in the file PATENTS.  All contributing project authors may
  *  be found in the AUTHORS file in the root of the source tree.
  */
 
@@ skipping 119 matching lines @@
   }
 
   virtual void Run() {
     // Do nothing.
   }
 
  private:
   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()() {
     Thread::Current()->ProcessMessages(50);
     return 24;
   }
 };
 
 // See: https://code.google.com/p/webrtc/issues/detail?id=2409
 TEST(ThreadTest, DISABLED_Main) {
@@ skipping 99 matching lines @@
   delete cthread;
   current_thread->WrapCurrent();
 }
 
 TEST(ThreadTest, Invoke) {
   // Create and start the thread.
   Thread thread;
   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);
   // Try calling bare functions.
   struct LocalFuncs {
     static int Func1() { return 999; }
     static void Func2() {}
   };
   EXPECT_EQ(999, thread.Invoke<int>(&LocalFuncs::Func1));
   thread.Invoke<void>(&LocalFuncs::Func2);
@@ skipping 121 matching lines @@
   Event invoke_started_;
   Thread* expected_thread_;
 };
 
 TEST_F(AsyncInvokeTest, FireAndForget) {
   AsyncInvoker invoker;
   // Create and start the thread.
   Thread thread;
   thread.Start();
   // Try calling functor.
-  bool called = false;
+  AtomicFlag called = false;
   invoker.AsyncInvoke<void>(&thread, FunctorB(&called));
   EXPECT_TRUE_WAIT(called, kWaitTimeout);
 }
 
 TEST_F(AsyncInvokeTest, WithCallback) {
   AsyncInvoker invoker;
   // Create and start the thread.
   Thread thread;
   thread.Start();
   // Try calling functor.
@@ skipping 49 matching lines @@
     // Wait for the call to begin.
     ASSERT_TRUE(invoke_started_.Wait(kWaitTimeout));
   }
   // Invoker is destroyed. Function should not execute.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 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));
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag2));
   // Because we haven't pumped messages, these should not have run yet.
   EXPECT_FALSE(flag1);
   EXPECT_FALSE(flag2);
   // Force them to run now.
   invoker.Flush(Thread::Current());
   EXPECT_TRUE(flag1);
   EXPECT_TRUE(flag2);
 }
 
 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),
                             5);
   invoker.AsyncInvoke<void>(Thread::Current(),
                             FunctorB(&flag2));
   // Because we haven't pumped messages, these should not have run yet.
   EXPECT_FALSE(flag1);
   EXPECT_FALSE(flag2);
   // Execute pending calls with id == 5.
@@ skipping 46 matching lines @@
 TEST_F(GuardedAsyncInvokeTest, KillThreadFireAndForget) {
   // Create and start the thread.
   scoped_ptr<Thread> thread(new Thread());
   thread->Start();
   scoped_ptr<GuardedAsyncInvoker> invoker;
   // Create the invoker on |thread|.
   thread->Invoke<void>(CreateInvoker(&invoker));
   // 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);
   EXPECT_FALSE(called);
 }
 
 // Test that we can call AsyncInvoke with callback after the thread died.
 TEST_F(GuardedAsyncInvokeTest, KillThreadWithCallback) {
   // Create and start the thread.
   scoped_ptr<Thread> thread(new Thread());
@@ skipping 10 matching lines @@
   // With thread gone, callback should be cancelled.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 // The remaining tests check that GuardedAsyncInvoker behaves as AsyncInvoker
 // when Thread is still alive.
 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);
 }
 
 TEST_F(GuardedAsyncInvokeTest, WithCallback) {
   GuardedAsyncInvoker invoker;
   // Try calling functor.
   SetExpectedThreadForIntCallback(Thread::Current());
   EXPECT_TRUE(invoker.AsyncInvoke(FunctorA(),
                                   &GuardedAsyncInvokeTest::IntCallback,
@@ skipping 44 matching lines @@
     // Wait for the call to begin.
     ASSERT_TRUE(invoke_started_.Wait(kWaitTimeout));
   }
   // Invoker is destroyed. Function should not execute.
   Thread::Current()->ProcessMessages(kWaitTimeout);
   EXPECT_EQ(0, int_value_);
 }
 
 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)));
   // Because we haven't pumped messages, these should not have run yet.
   EXPECT_FALSE(flag1);
   EXPECT_FALSE(flag2);
   // Force them to run now.
   EXPECT_TRUE(invoker.Flush());
   EXPECT_TRUE(flag1);
   EXPECT_TRUE(flag2);
 }
 
 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)));
   // Because we haven't pumped messages, these should not have run yet.
   EXPECT_FALSE(flag1);
   EXPECT_FALSE(flag2);
   // Execute pending calls with id == 5.
   EXPECT_TRUE(invoker.Flush(5));
   EXPECT_TRUE(flag1);
   EXPECT_FALSE(flag2);
@@ skipping 22 matching lines @@
 };
 
 TEST_F(ComThreadTest, ComInited) {
   Thread* thread = new ComThread();
   EXPECT_TRUE(thread->Start());
   thread->Post(this, 0);
   EXPECT_TRUE_WAIT(done_, 1000);
   delete thread;
 }
 #endif