Re: 148 - ThreadLocal and Mutex for pthread

[vlad...@gmail.com]

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?

http://codereview.appspot.com/129067

[Fazeka...@gmail.com]

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

http://codereview.appspot.com/129067

[vlad...@gmail.com]

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:

> 7 FAILED TESTS

> PASS

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.

http://codereview.appspot.com/129067

[Fazeka...@gmail.com]

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.

http://codereview.appspot.com/129067

[vlad...@gmail.com]

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.

http://codereview.appspot.com/129067

[Fazeka...@gmail.com]

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()

http://codereview.appspot.com/129067

[vlad...@gmail.com]

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().

http://codereview.appspot.com/129067

[Fazeka...@gmail.com]

-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?

http://codereview.appspot.com/129067