148 - ThreadLocal and Mutex for pthread

[mfazekas, 2009-10-14 20:32:49]

http://codereview.appspot.com/129067/diff/1/3
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/1/3#newcode755
Line 755: 
I'm throwing an exception when the pthread api returns an error, is it ok, or
should i use GTEST_CHECK_ instead?

http://codereview.appspot.com/129067/diff/1/3#newcode799
Line 799: ImplMutex& impl() { return lazyInit(); }
The stuff bellow is for handling the case where two threads entering the
lazyInit() at the same time. It's only possible if we start threads before main
- from static globals or we use Mutex(NO_CONSTRUCTOR_NEEDED_FOR_STATIC_MUTEX) in
a static local. Shall we handle this case at all? (There is also a large/slow
testcase testing for this.)

http://codereview.appspot.com/129067/diff/1/2
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/129067/diff/1/2#newcode6609
Line 6609: TEST(MutexTest,LazyInitOnStaticMutexShouldBeThreadSafe) {
The stuff bellow is for handling the case where two threads entering the
lazyInit() at the same time. It's only possible if we start threads before main
- from static globals or we use Mutex(NO_CONSTRUCTOR_NEEDED_FOR_STATIC_MUTEX) in
a static local. Shall we handle this case at all? (There is also a large/slow
testcase testing for this.)

http://codereview.appspot.com/129067/diff/1/2#newcode7113
Line 7113: }
This file is huge, shouldn't we split it?

[florianlink, 2009-10-15 07:38:20]

I found a small race that is probably only relevant for the unit testing.

http://codereview.appspot.com/129067/diff/1/4
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/1/4#newcode113
Line 113: owner_ = 0;
Setting the owner_ to 0 here is too late, it is a race with another thread being
in lock(). You need to move owner_ = 0 before line 110

[mfazekas, 2009-10-15 14:33:07]

http://codereview.appspot.com/129067/diff/1/4
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/1/4#newcode113
Line 113: owner_ = 0;
Yes, the owner_ = 0 should be moved before pthread_mutex_unlock.

[Vlad, 2009-10-20 22:40:20]

Hi Miklos,

Thanks for the patch. This is a big one! There are some comments below and I
have also committed tests in gtest_stress_test to svn. Please move Thread to
src/gtest-internal-inl.h as described below and hook up gtest_stress_test.cc
with your primitives - it provides a good exercise for them.

Also, our project follows fairly strict coding style requirements, described in
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml. After we work
out the substantial issues I can take the patch and work out all the stylistic
issues myself, if you like.

Thanks,
Vlad

http://codereview.appspot.com/129067/diff/1/3
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/1/3#newcode755
Line 755: 
On 2009/10/14 20:32:49, mfazekas wrote:
> I'm throwing an exception when the pthread api returns an error, is it ok, or
> should i use GTEST_CHECK_ instead?

Please use GTEST_CHECK_. As Google Test has to work without exceptions enabled,
it should not throw.

http://codereview.appspot.com/129067/diff/1/3#newcode780
Line 780: class Mutex {
Can you put Mutex implementation into gtest_port.cc so that there is no need for
gtest_port.h to #include <pthread.h>?

http://codereview.appspot.com/129067/diff/1/3#newcode799
Line 799: ImplMutex& impl() { return lazyInit(); }
On 2009/10/14 20:32:49, mfazekas wrote:
> The stuff bellow is for handling the case where two threads entering the
> lazyInit() at the same time.

Why are you doing lazy initialization? It postpones mutex initialization to the
point where potential races are unavoidable and you have to write a lot of code
to work around the issue.

> It's only possible if we start threads before main
> - from static globals or we use Mutex(NO_CONSTRUCTOR_NEEDED_FOR_STATIC_MUTEX)
in
> a static local. Shall we handle this case at all? (There is also a large/slow
> testcase testing for this.)

As I have in the email reply, this mutex implementation is specific to Google
Test, and if we can get away with reduced complexity at the cost of generality,
we should do it - as long as it doesn't affect our correctness. Please put in
comments documenting the limitation instead.

http://codereview.appspot.com/129067/diff/1/3#newcode823
Line 823: template <typename T>
Please comment here what are the requirements for the types that ThreadLocal can
handle. Also we have to put some thought into whether this will be consistent
with the eventual Win32 implementation of ThreadLocal.

http://codereview.appspot.com/129067/diff/1/3#newcode870
Line 870: size_t GetThreadCount();
If you don't implement GetThreadCount for pthreads, please say so in the
comments. I.e.

pthreads doesn't provide support for counting threads in the process so we use
generic implementation that returns 0.

http://codereview.appspot.com/129067/diff/1/4
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/1/4#newcode99
Line 99: GTEST_CHECK_ (owner_ == pthread_self()) << "AssertHeld failed.";
Please provide a more informative message, such as

"Current thread is not holding mutex " << this;

As this will be the last printout from the process, it makes sense to provide
any information that can be useful for analysis.

http://codereview.appspot.com/129067/diff/1/4#newcode162
Line 162: #endif
#endif  // GTEST_HAS_PTHREAD

http://codereview.appspot.com/129067/diff/1/2
File test/gtest_unittest.cc (right):

http://codereview.appspot.com/129067/diff/1/2#newcode6442
Line 6442: class NoCopyConstructor {
Constructs defined in gtest-port.h and gtest-port.cc have their own test file:
gtest-port_test.cc. Please move these tests there; I will provide more comments
when that is done to avoid losing context.

http://codereview.appspot.com/129067/diff/1/2#newcode6475
Line 6475: Thread(void* (func)(void*),void*param) : thread_(), running_(false) {
As Thread is also needed by gtest_strewss_test, can you please move it to a
place where it will be available to both gtest-port_test and gtest_stress_test?
The end of src/gtest-internal-inl.h (still in the testing::internal namespace)
is an appropriate place.

http://codereview.appspot.com/129067/diff/1/2#newcode6609
Line 6609: TEST(MutexTest,LazyInitOnStaticMutexShouldBeThreadSafe) {
On 2009/10/14 20:32:49, mfazekas wrote:
> The stuff bellow is for handling the case where two threads entering the
> lazyInit() at the same time. It's only possible if we start threads before
main
> - from static globals or we use Mutex(NO_CONSTRUCTOR_NEEDED_FOR_STATIC_MUTEX)
in
> a static local. Shall we handle this case at all? (There is also a large/slow
> testcase testing for this.)

I think we don't need support for mutexes initialized before main() for now.

[mfazekas, 2009-11-01 17:16:28]

This is a new patch that fixed these issues since the last patch:
- uses GTEST_CHECK_ instead of pthread_exception
- still uses lazyInit for static mutexes but don't handle the race - this
greatly simplifies the code. This means that static mutexes can be used before
main, and they are safe if they are used from a single thread (main thread).
- Thread changed to ThreadWithParam and is now in internal-iml
- owner_ in MutexImpl for AssertHold is now correctly protected by the mutex
- other smaller fixes/cleanup according to review notes

http://codereview.appspot.com/129067/diff/4001/5004
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/4001/5004#newcode367
Line 367: 
I'm still including pthread, - one implementation that avoids that would be to
make MutexImpl, and ThreadLocalPointerImpl interfaces - abstract classes, and
PthreadMutexImpl would implement them in gtest_port.cc (~bridge pattern). I'm
happy to implement this, but I've got a bit uncertain when i saw that we do have
#include <regex.h> as well.

http://codereview.appspot.com/129067/diff/4001/5004#newcode792
Line 792: MutexImpl& lazyInit();
Still uses lazyInit, so that Mutex is useable before main. But we don't handle
races in lazyInit - but it should be safe as long as we don't call it from
non-main thread before main started.

http://codereview.appspot.com/129067/diff/4001/5004#newcode822
Line 822: };
Should be fairly easy to implement with win32 using tss. The only issue is
calling deleteFunction on thread exit - which is probably requires DllMain with
DLL_THREAD_DEATTACH.

http://codereview.appspot.com/129067/diff/4001/5001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/4001/5001#newcode861
Line 861: TEST(MutexTest, LazyInitOnStaticMutexShouldDetectRace) {
This test is fragile. It tests the assert which detects race in Mutex lazyInit.
But that detection is not 100% - so maybe i should just remove this test.

http://codereview.appspot.com/129067/diff/4001/5002
File test/gtest_stress_test.cc (left):

http://codereview.appspot.com/129067/diff/4001/5002#oldcode56
Line 56: // thread safe, implement ThreadWithParam with the following interface:
The result of stress_tests seems to be FAIL - even though they seems to generete
the expected number of failures.

http://codereview.appspot.com/129067/diff/4001/5003
File test/gtest_unittest.cc (left):

http://codereview.appspot.com/129067/diff/4001/5003#oldcode6571
Line 6571: 
Moved the ThreadLocalTest tests to port-test

[Vlad, 2009-11-04 10:21:11]

Hi Miklos,

This is shaping up. :-) Here are some more comments. 

It is desirable to move #include <pthread.h> to .cc file but feel free not to do
it if it proves too hard.

- Vlad

http://codereview.appspot.com/129067/diff/4001/5004
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/4001/5004#newcode367
Line 367: 
On 2009/11/01 17:16:29, mfazekas wrote:
> I'm still including pthread, - one implementation that avoids that would be to
> make MutexImpl, and ThreadLocalPointerImpl interfaces - abstract classes, and
> PthreadMutexImpl would implement them in gtest_port.cc (~bridge pattern). I'm
> happy to implement this, but I've got a bit uncertain when i saw that we do
have
> #include <regex.h> as well.

You can use the pimpl idiom. Use a pointer to a forward declared class:

// In the .h file:
class Mutex {
 public:
  // All methods are implemented in the .cc file
  // by delegating to impl_.
  void Lock();

 private:
  class PthreadMutex* impl_;
};

// In the .cc file:
#include <pthread.h>
class PthreadMutex {
  // Implements pthread-based mutex.
};

Mutex::Mutex() : impl_(new PthreadMutex) {}
Mutex::~Mutex() { delete impl_; }
void Mutex::Lock() { impl_->Lock(); }

regex.h is waiting for its turn to get moved into the .cc file. :-)

http://codereview.appspot.com/129067/diff/4001/5004#newcode792
Line 792: MutexImpl& lazyInit();
On 2009/11/01 17:16:29, mfazekas wrote:
> Still uses lazyInit, so that Mutex is useable before main. But we don't handle
> races in lazyInit - but it should be safe as long as we don't call it from
> non-main thread before main started.

I think you should handle races in lazyInit. The mutex may be created well
before main but used only after main starts. At that point you may already have
multiple threads and race conflicts. Just avoid the double-checked locking
there. :-)

http://codereview.appspot.com/129067/diff/4001/5001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/4001/5001#newcode698
Line 698: TEST(ThreadLocalTest, DefaultConstructor) {
ThreadLocal is defined only under GTEST_IS_THREADSAFRE=1.

http://codereview.appspot.com/129067/diff/4001/5001#newcode861
Line 861: TEST(MutexTest, LazyInitOnStaticMutexShouldDetectRace) {
On 2009/11/01 17:16:29, mfazekas wrote:
> This test is fragile. It tests the assert which detects race in Mutex
lazyInit.
> But that detection is not 100% - so maybe i should just remove this test.

You should guard lazy initialization with a mutex.

http://codereview.appspot.com/129067/diff/4001/5002
File test/gtest_stress_test.cc (left):

http://codereview.appspot.com/129067/diff/4001/5002#oldcode56
Line 56: // thread safe, implement ThreadWithParam with the following interface:
On 2009/11/01 17:16:29, mfazekas wrote:
> The result of stress_tests seems to be FAIL - even though they seems to
generete
> the expected number of failures.

Does he executable return exit code of 0?

[mfazekas, 2009-11-04 17:40:45]

Vlad,

See comments inline. I'll move #include <pthread.h> out of gtest-port.h.

Miklos

http://codereview.appspot.com/129067/diff/4001/5004
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/4001/5004#newcode367
Line 367: 
On 2009/11/04 10:21:12, Vlad wrote:
> On 2009/11/01 17:16:29, mfazekas wrote:
> > I'm still including pthread, - one implementation that avoids that would be
to
> > make MutexImpl, and ThreadLocalPointerImpl interfaces - abstract classes,
and
> > PthreadMutexImpl would implement them in gtest_port.cc (~bridge pattern).
> You can use the pimpl idiom. Use a pointer to a forward declared class:

You're right that in the Mutex case the pimpl is enough. I was worried about the
templated ThreadLocal. In that case i can put pimpl inside
ThreadLocalPointerImpl or make ThreadLocalPointerImpl an abstract class -
interface.

http://codereview.appspot.com/129067/diff/4001/5004#newcode792
Line 792: MutexImpl& lazyInit();
On 2009/11/04 10:21:12, Vlad wrote:
> I think you should handle races in lazyInit. The mutex may be created well
> before main but used only after main starts. At that point you may already
have
> multiple threads and race conflicts. Just avoid the double-checked locking
> there. :-)

We do call lazyInit from the constructor, which will be called before main. So
any use after main will see the mutex already initialized I'll document that.

The problem with handing races in lazyInit() is that it needs a global mutex to
protect - and that global mutex needs to be intialized with pthread_once. It's
possible just increases complexity.

http://codereview.appspot.com/129067/diff/4001/5001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/4001/5001#newcode698
Line 698: TEST(ThreadLocalTest, DefaultConstructor) {
On 2009/11/04 10:21:12, Vlad wrote:
> ThreadLocal is defined only under GTEST_IS_THREADSAFRE=1. 

Ther is a dummy thread local impl in ! GTEST_IS_THREADSAFRE - these test should
pass on that implementation too.

http://codereview.appspot.com/129067/diff/4001/5002
File test/gtest_stress_test.cc (left):

http://codereview.appspot.com/129067/diff/4001/5002#oldcode56
Line 56: // thread safe, implement ThreadWithParam with the following interface:
On 2009/11/04 10:21:12, Vlad wrote:
> On 2009/11/01 17:16:29, mfazekas wrote:
> > The result of stress_tests seems to be FAIL - even though they seems to
> generete
> > the expected number of failures.
> 
> Does he executable return exit code of 0?

[==========] 7 tests from 6 test cases ran. (3389 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 7 tests, listed below:
[  FAILED  ] StressTest.CanUseScopedTraceAndAssertionsInManyThreads
[  FAILED  ]
NoFatalFailureTest.ExpectNoFatalFailureIgnoresFailuresInOtherThreads
[  FAILED  ]
NoFatalFailureTest.AssertNoFatalFailureIgnoresFailuresInOtherThreads
[  FAILED  ] FatalFailureTest.ExpectFatalFailureIgnoresFailuresInOtherThreads
[  FAILED  ] FatalFailureOnAllThreadsTest.ExpectFatalFailureOnAllThreads
[  FAILED  ]
NonFatalFailureTest.ExpectNonFatalFailureIgnoresFailuresInOtherThreads
[  FAILED  ] NonFatalFailureOnAllThreadsTest.ExpectNonFatalFailureOnAllThreads

 7 FAILED TESTS

PASS

Debugger stopped.
Program exited with status value:0.

I guess i've just misinterpreted the results and this test has passed

[Vlad, 2009-11-05 15:01:24]

Miklos,

Are your latest changes uploaded here? I don't see the ThreadLocal changes.

Thanks,
Vlad

http://codereview.appspot.com/129067/diff/4001/5004
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/4001/5004#newcode792
Line 792: MutexImpl& lazyInit();
On 2009/11/04 17:40:45, mfazekas wrote:
> On 2009/11/04 10:21:12, Vlad wrote:
> > I think you should handle races in lazyInit. The mutex may be created well
> > before main but used only after main starts. At that point you may already
> have
> > multiple threads and race conflicts. Just avoid the double-checked locking
> > there. :-)
> 
> We do call lazyInit from the constructor, which will be called before main. So
> any use after main will see the mutex already initialized I'll document that.
> 
> The problem with handing races in lazyInit() is that it needs a global mutex
to
> protect - and that global mutex needs to be intialized with pthread_once. It's
> possible just increases complexity.
> 

It is possible to catch the races here with an assertion as an alternative to
handling them using locking.
The problem with this approach is that stopping the test to report the race
leads to users' tests becoming flaky. This is a very undesirable property, as
flaky tests lower the confidence of the users in their code under test. So I
think we should bite the bullet and put locking in.

Here is a relatively simple implementation of lazy initialization which doesn't
incur the locking penalty for dynamicaly created mutexes. Feel free to use
whatever ideas you like. :-)

class Mutex {
 public:
  Mutex();
  Mutex(NoConstructorNeededSelector dummy);

  // Lock and Unlock defined here.

 private:
  class PthreadMutex* impl();

  const bool eagerly_initialized_;
  PthreadMutex* impl_;
};

Mutex::Mutex()
    : eagerly_initlialized(true),
      impl_(new PthreadMutex) {}

Mutex::Mutex(NoConstructorNeededSelector /*dummy*/) {}

PthreadMutex* Mutex::impl() {
  if (!eagerly_initialized_) {
    PthreadMutex::global_mutex()->Lock();
    if (impl_ == NULL)
      impl_ = new PthreadMutex;
    PthreadMutex::global_mutex()->Unlock();
  }
  return impl_;
}

class PthreadMutex {
 public:
  // Constructor, destructor, Lock, and Unlock go here.

 private:
  friend classs Mutex;

  static PthreadMutex* global_mutex() {
    pthread_once(&once_control_, &InitGlobalMutex);
    return global_mutex_;
  }

  static void InitGlobalMutex() {
    global_mutex_ = new PthreadMutex;
  }

  static PthreadMutex* global_mutex_;
  static pthread_once_t once_control_;
};

PthreadMutex* PthreadMutex::global_mutex = NULL;
pthread_once_t PthreadMutex::once_control_ = PTHREAD_ONCE_INIT;

http://codereview.appspot.com/129067/diff/4001/5002
File test/gtest_stress_test.cc (left):

http://codereview.appspot.com/129067/diff/4001/5002#oldcode56
Line 56: // thread safe, implement ThreadWithParam with the following interface:
On 2009/11/04 17:40:45, mfazekas wrote:
> On 2009/11/04 10:21:12, Vlad wrote:
> > On 2009/11/01 17:16:29, mfazekas wrote:
> > > The result of stress_tests seems to be FAIL - even though they seems to
> > generete
> > > the expected number of failures.
> > 
> > Does he executable return exit code of 0?
> 
> [==========] 7 tests from 6 test cases ran. (3389 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 7 tests, listed below:
> [  FAILED  ] StressTest.CanUseScopedTraceAndAssertionsInManyThreads
> [  FAILED  ]
> NoFatalFailureTest.ExpectNoFatalFailureIgnoresFailuresInOtherThreads
> [  FAILED  ]
> NoFatalFailureTest.AssertNoFatalFailureIgnoresFailuresInOtherThreads
> [  FAILED  ] FatalFailureTest.ExpectFatalFailureIgnoresFailuresInOtherThreads
> [  FAILED  ] FatalFailureOnAllThreadsTest.ExpectFatalFailureOnAllThreads
> [  FAILED  ]
> NonFatalFailureTest.ExpectNonFatalFailureIgnoresFailuresInOtherThreads
> [  FAILED  ] NonFatalFailureOnAllThreadsTest.ExpectNonFatalFailureOnAllThreads
> 
>  7 FAILED TESTS
> 
> PASS
> 
> Debugger stopped.
> Program exited with status value:0.
> 
> I guess i've just misinterpreted the results and this test has passed

If you build gtest using SCons, you can run tests using the run_tests.py script:

./run_tests.py gtest_stress_test

It will report the ultimate pass/fail result clearly.

[mfazekas, 2009-11-06 00:53:35]

Fixes: 
- fixed potential race using global mutex (wasn't really relevant as this could
happen only if threads starts before main. But it was confusing and now we dont
need to explain this non threadsafe situation in comments, so in the end this is
probably simpler)
- removed pthread.h from gtest-port.h by using pimpl
- do not destruct static mutexes as they might be used after their destructor

http://codereview.appspot.com/129067/diff/10001/9013
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/10001/9013#newcode140
src/gtest-port.cc:140: if (type_ != StaticMutex)
We need to leak static mutexes, they should be valid before their constructor
and after their destructor.

[Vlad, 2009-11-07 21:42:03]

http://codereview.appspot.com/129067/diff/10001/9011
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/10001/9011#newcode757
include/gtest/internal/gtest-port.h:757: class LockGuard {
Why have an extra class for GTestMutexLock's functionality and delegate here?
GTestMutexLock can be made a friend if you want to keep Lock and Unlock private.

http://codereview.appspot.com/129067/diff/10001/9011#newcode795
include/gtest/internal/gtest-port.h:795: protected:
Are you going to inherit from ThreadLocalPointer?

http://codereview.appspot.com/129067/diff/10001/9011#newcode809
include/gtest/internal/gtest-port.h:809: // T must either have a default or copy
constructor - but not both.
Your comment reads as if T cannot have both default and copy constructor? Is
this correct?

http://codereview.appspot.com/129067/diff/10001/9013
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/10001/9013#newcode110
src/gtest-port.cc:110: class GlobalMutexImpl {
Why introduce an extra class to initialize the global mutex?

http://codereview.appspot.com/129067/diff/10001/9013#newcode120
src/gtest-port.cc:120: GTEST_CHECK_(err == 0) << "pthread_once failed with:" <<
err;
Why an extra CHECK here? Documentation for pethread_once
(http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_once.html) says that
the function only fails if it is given bad parameters.

http://codereview.appspot.com/129067/diff/10001/9013#newcode135
src/gtest-port.cc:135: Mutex::Mutex(NoConstructorNeededSelector /*dummy*/) {
impl(); }
I have missed the part about invoking Mutex::init in Mutex's static constructor.
Putting it here ensures a fully constructed mutex by the time main() starts. As
gtest does not start any threads before main(), this makes locking in init()
redundant. My apologies for making a premature conclusion.

http://codereview.appspot.com/129067/diff/10001/9013#newcode140
src/gtest-port.cc:140: if (type_ != StaticMutex)
On 2009/11/06 00:53:35, mfazekas wrote:
> We need to leak static mutexes, they should be valid before their constructor
> and after their destructor.

Yep, you are right about it.

http://codereview.appspot.com/129067/diff/10001/9008
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/10001/9008#newcode746
test/gtest-port_test.cc:746: TEST(MutexTest,
AssertHeldShouldAssertWhenNotLocked) {
By convention, death test names should end wih DeathTest.

http://codereview.appspot.com/129067/diff/10001/9008#newcode763
test/gtest-port_test.cc:763: memcpy(&counter_, &temp, sizeof(temp));
A compiler is likely to optimize these four lines into the single counter++
statement which can evade the race.

Something like this is much more likely to trigger a race condition: 

volatile int temp = counter_ + 1;

// Sleeps up to 1 second.
timespec time;
time.tv_sec = 0;
time.tv_nsec = random->Generate(1000) * 1000L * 1000;
nanosleep(&time, NULL);

counter_ = temp;

http://codereview.appspot.com/129067/diff/10001/9008#newcode827
test/gtest-port_test.cc:827: #if GTEST_HAS_PTHREAD
As we will define ThreadWithParam on every platform where we define
GTEST_IS_THREADSAFE, we should get rid of the conditional compilation here.

[mfazekas, 2009-11-08 22:46:46]

Vlad,

Thanks for the comments. I've fixed some of the issues and commented others
where i disagree.

Regards,
Miklos

http://codereview.appspot.com/129067/diff/10001/9013
File src/gtest-port.cc (right):

http://codereview.appspot.com/129067/diff/10001/9013#newcode110
src/gtest-port.cc:110: class GlobalMutexImpl {
On 2009/11/07 21:42:03, Vlad wrote:
> Why introduce an extra class to initialize the global mutex?
MutexImpl implements the mutex functionality for Mutex. GlobalMutexImpl
implements the global lock needed for the synchronization for the lazyInit in
static Mutex-es. They can be one class but i don't see why that would be better.
For example if we decide we don't need the sync in lazyInit we can just remove
GlobalMutexImpl without affecting MutexImpl. Also a platform without
pthread_once like logic might impement GlobalLockImpl using splinlock.

http://codereview.appspot.com/129067/diff/10001/9013#newcode120
src/gtest-port.cc:120: GTEST_CHECK_(err == 0) << "pthread_once failed with:" <<
err;
On 2009/11/07 21:42:03, Vlad wrote:
> Why an extra CHECK here? Documentation for pethread_once
> (http://www.opengroup.org/onlinepubs/007908799/xsh/pthread_once.html) says
that
> the function only fails if it is given bad parameters.
Why not? It's better to be paranoid. If a 3rd party code returns an error code
we better check it.

http://codereview.appspot.com/129067/diff/10001/9013#newcode135
src/gtest-port.cc:135: Mutex::Mutex(NoConstructorNeededSelector /*dummy*/) {
impl(); }
On 2009/11/07 21:42:03, Vlad wrote:
> I have missed the part about invoking Mutex::init in Mutex's static
constructor.
> Putting it here ensures a fully constructed mutex by the time main() starts.
As
> gtest does not start any threads before main(), this makes locking in init()
> redundant. My apologies for making a premature conclusion.
I'd vote for keeping the global lock in the lazyInit. With this +20 lines of
code we can save some confusing comments explaining when this race can happen
and why this doesn't affect users.

http://codereview.appspot.com/129067/diff/10008/10012
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/10008/10012#newcode806
include/gtest/internal/gtest-port.h:806: // The single param constructor
requires a copy contructor from T.
On 2009/11/07 21:42:03, Vlad wrote:
> Your comment reads as if T cannot have both default and copy constructor? Is
> this correct?

Reworded comments hopefully it's better now.

http://codereview.appspot.com/129067/diff/10008/10009
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/10008/10009#newcode772
test/gtest-port_test.cc:772: counter_ = temp_+1;
On 2009/11/07 21:42:03, Vlad wrote:
> Something like this is much more likely to trigger a race condition: 
> 
> time.tv_nsec = random->Generate(1000) * 1000L * 1000;
> nanosleep(&time, NULL);
> 
Thanks for the tip. I think that putting nanosleep into gtest is not trivial as
we have to introduce GTEST_HAS_NANOSLEEP for portability. So that's the reason
for the not so efficient ForceContextSwitch()

[Vlad, 2009-11-10 03:24:11]

Just a couple more things.

http://codereview.appspot.com/129067/diff/10008/10009
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/10008/10009#newcode770
test/gtest-port_test.cc:770: temp_ = counter_;
It is unclear why temp needs to be a member of AtomicCounterWithMutex. If you
deem this important, please add a comment in the code on why it is.

http://codereview.appspot.com/129067/diff/10008/10009#newcode772
test/gtest-port_test.cc:772: counter_ = temp_+1;
On 2009/11/08 22:46:47, mfazekas wrote:
> On 2009/11/07 21:42:03, Vlad wrote:
> > Something like this is much more likely to trigger a race condition: 
> > 
> > time.tv_nsec = random->Generate(1000) * 1000L * 1000;
> > nanosleep(&time, NULL);
> > 
> Thanks for the tip. I think that putting nanosleep into gtest is not trivial
as
> we have to introduce GTEST_HAS_NANOSLEEP for portability. So that's the reason
> for the not so efficient ForceContextSwitch() 

Nanosleep was just an example; we don't have to use it. Essentially all modern
systems have sub-second sleep. Most *nix systems have usleep
(http://www.opengroup.org/onlinepubs/000095399/functions/usleep.html), and
Windows has Sleep. I think you can just use usleep where. It is a  reasonable
assumption that all phtread-supporting platforms implement it. When we encounter
a platform that doesn't or when we port the multithreading support to Windows,
we can abstract it into testing::internal::SleepMilliseconds().

[mfazekas, 2009-11-12 06:24:51]

-Small fixes in port_test: i'm using usleep instead of starting/joining a thread
for forcing a context switch, also removed the unnecessary temp_.

http://codereview.appspot.com/129067/diff/11001/12001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/11001/12001#newcode923
test/gtest-port_test.cc:923:
counter().push_back(ThreadWithParam<int>::CurrentThreadId());
Do your implementation of ThreadWithParam has threadid? And can it be added?
I need it for the test bellow, which verifies that ThreadLocal's destructors are
executed on proper thread.
I don't really like that for CurrentThreadId() is called like this -
ThreadWithParam<int>::CurrentThreadId() - eg. i have to specify a template
parameter. Any better ideas?

[Vlad, 2009-11-15 22:05:56]

I think we are almost done here. Once we resolve the last issue I take the code,
touch it up to make it conform to Google style guide. It will then have to pass
through our internal review so it may mutate further. Once that is done, it will
be committed to SVN. Sounds OK?


- Vlad

http://codereview.appspot.com/129067/diff/11001/12001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/11001/12001#newcode923
test/gtest-port_test.cc:923:
counter().push_back(ThreadWithParam<int>::CurrentThreadId());
On 2009/11/12 06:24:51, mfazekas wrote:
> Do your implementation of ThreadWithParam has threadid? And can it be added?

Sure. The goal of ThreadWithParam is support for testing threading support, so
it's the perfect place.

> I need it for the test bellow, which verifies that ThreadLocal's destructors
are
> executed on proper thread.
> I don't really like that for CurrentThreadId() is called like this -
> ThreadWithParam<int>::CurrentThreadId() - eg. i have to specify a template
> parameter. Any better ideas?

It just crossed my mind that the other platforms (specifically, Win32/TLS) do
not support deallocating per-thread resources automatically when the thread
exits. They make it the responsibility of the thread itself. In that light, we
cannot provide the cross-platform guarantee of deleting objects on the thread
where they are held. We can only provide a very weak guarantee that the objects
will be deleted subsequently before the program terminates.

Given that we should remove this test. I may be wrong; if you see a way of
providing cross-platform support for on-thread deletion, please let me know and
keep the test.

[mfazekas, 2009-11-15 23:52:58]

It sounds great that we're getting close to the finish. See comments bellow
about the last issue.

If you agree i'll implement a TestCase that only verifies the weaker semantics:
thread locals deleted no later than ~ThreadLocal. We can implement that on
windows and i can also get a rid of GetCurrentThreadId. (Which should be fixed
to call pthread_equal from operator== anyway)

Regards,
Miklos

http://codereview.appspot.com/129067/diff/11001/12001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/11001/12001#newcode923
test/gtest-port_test.cc:923:
counter().push_back(ThreadWithParam<int>::CurrentThreadId());
On 2009/11/15 22:05:56, Vlad wrote:
> On 2009/11/12 06:24:51, mfazekas wrote:
> > I need it for the test bellow, which verifies that ThreadLocal's destructors
> are
> > executed on proper thread.
> 
> It just crossed my mind that the other platforms (specifically, Win32/TLS) do
> not support deallocating per-thread resources automatically when the thread
> exits. They make it the responsibility of the thread itself. In that light, we
> cannot provide the cross-platform guarantee of deleting objects on the thread
> where they are held. We can only provide a very weak guarantee that the
objects
> will be deleted subsequently before the program terminates.
> 
> Given that we should remove this test. I may be wrong; if you see a way of
> providing cross-platform support for on-thread deletion, please let me know
and
> keep the test.

On Windows i think these are the possible implementations:
a.) try to detect thread termination: DllExit and DLL_THREAD_DETACH works if
gtest is a .dll. It's not a .dll currently.  (Also there seems to be FlsCallback
but that is not available on XP)
b.) introduce a method that the user has to call from the thread before it's
termination
c.) call delete function from ~ThreadLocal<T>.

You're probably right and the cleaning up from ~ThreadLocal is the best for
win32.

On pthread i think we should probably continue to use the cleanup from thread
exit - as that is simpler to implement.

So we can either remove the current test or put it into #ifdef PTHREAD. And we
need another test which tests that the objects are destroyed no later than
~ThreadLocal, and does not check which thread we've executed the destruction
from.

[Vlad, 2009-11-16 03:21:45]

You are right, it looks like we can guarantee per-thread object destruction by
the time ~ThreadLocal finishes. A test for this semantics is the right thing.

http://codereview.appspot.com/129067/diff/11001/12001
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/11001/12001#newcode923
test/gtest-port_test.cc:923:
counter().push_back(ThreadWithParam<int>::CurrentThreadId());
On 2009/11/15 23:52:58, mfazekas wrote:
> On 2009/11/15 22:05:56, Vlad wrote:
> > On 2009/11/12 06:24:51, mfazekas wrote:
> > > I need it for the test bellow, which verifies that ThreadLocal's
destructors
> > are
> > > executed on proper thread.
> > 
> > It just crossed my mind that the other platforms (specifically, Win32/TLS)
do
> > not support deallocating per-thread resources automatically when the thread
> > exits. They make it the responsibility of the thread itself. In that light,
we
> > cannot provide the cross-platform guarantee of deleting objects on the
thread
> > where they are held. We can only provide a very weak guarantee that the
> objects
> > will be deleted subsequently before the program terminates.
> > 
> > Given that we should remove this test. I may be wrong; if you see a way of
> > providing cross-platform support for on-thread deletion, please let me know
> and
> > keep the test.
> 
> On Windows i think these are the possible implementations:
> a.) try to detect thread termination: DllExit and DLL_THREAD_DETACH works if
> gtest is a .dll. It's not a .dll currently.  (Also there seems to be
FlsCallback
> but that is not available on XP)
> b.) introduce a method that the user has to call from the thread before it's
> termination
> c.) call delete function from ~ThreadLocal<T>.
> 
> You're probably right and the cleaning up from ~ThreadLocal is the best for
> win32.
> 

> On pthread i think we should probably continue to use the cleanup from thread
> exit - as that is simpler to implement.
> 
We should have consistent documented cross-platform behavior.  If I understand
correctly, per-thread objects on all threads will still be cleaned up upon
ThreadLocal's destruction under pthreads. If this is the case, I don't see a
problem cleaning them earlier if threads exit. We define ThreadLocal's behavior
as  'a per-thread object managed by a ThreadLocal instance for a thread will
exist at least for the runtime of the thread, unless the ThreadLocal managing is
alive. The managed object will exist no longer then the ThreadLocal object
managing it.' Such definition gives us enough flexibility to implement
destruction both ways under pthreads.

> So we can either remove the current test or put it into #ifdef PTHREAD. And we
> need another test which tests that the objects are destroyed no later than
> ~ThreadLocal, and does not check which thread we've executed the destruction
> from.

The tests essentially serve as additional documentation. As we document the
behavior of ThreadLocal to delete per-thread objects no later then its own
destruction, tests should reflect that. Given that, we should replace this test
with the one that verifies per thread object  destruction before ~ThreadLocal
finishes.

[mfazekas, 2009-11-17 00:08:36]

Changes:
- added comment about lifetime of threadlocals
- removed ThreadId stuff, modified the test of threadlocals lifetime

Regards,
Miklos

http://codereview.appspot.com/129067/diff/13024/11038
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/13024/11038#newcode809
include/gtest/internal/gtest-port.h:809: // not alive. The managed object will
exist no longer then the ThreadLocal
I've inserted a not here, as the original was - "unless the ThreadLocal managing
is alive"

http://codereview.appspot.com/129067/diff/13024/11035
File test/gtest-port_test.cc (right):

http://codereview.appspot.com/129067/diff/13024/11035#newcode952
test/gtest-port_test.cc:952: ASSERT_EQ(2, CountedDestructor::counter());
This 2 is not really nice, but i did not have a better idea at the moment.

[Vlad, 2009-11-18 09:32:09]

Hi Miklos,

Cool, we are there. :-) I'll update it to conform to the style guide now and
commit into svn. It may take some time; I will let you know when it's in.

Cheers,
Vlad

http://codereview.appspot.com/129067/diff/13024/11038
File include/gtest/internal/gtest-port.h (right):

http://codereview.appspot.com/129067/diff/13024/11038#newcode809
include/gtest/internal/gtest-port.h:809: // not alive. The managed object will
exist no longer then the ThreadLocal
On 2009/11/17 00:08:36, mfazekas wrote:
> I've inserted a not here, as the original was - "unless the ThreadLocal
managing
> is alive"

Cool, thanks.