Re: Question regarding thread-safety of GoogleTest
Zhanyong Wan (λx.x x)
On Thu, Oct 8, 2009 at 9:42 AM, Florian Link <flori...@gmail.com> wrote:
> Hi,
>
> we started using GoogleTest in our company and I really like it a lot.
> I noticed that include/gtest/internal/gtest-port.h contains some stubs
> for Mutex/etc. to make GoogleTest thread-safe.
> I wondered if you plan to provide a cross-platform implementation for
> these classes in one of the next releases, e.g. using boost.threads?
It's on our radar. You can star this issue
(http://code.google.com/p/googletest/issues/detail?id=148) to vote for
it and get notifications on its progress.
> As a sidenote, I would suggest that you only enable the stubs if
> GTEST_IS_THREADSAFE is 0.
> This would allow people who do their own threading implementation to
> use the original gtest-port.h file instead of
> patching it on each new google test release.
>
> I will probably implement a boost.threads version and would be willing
> to share it with you.
That would be nice!
One thing to note: we would like to avoid depending on Boost if
possible, as most users really don't like having to manage this
heavy-weight dependency. Therefore it's best if the implementation
doesn't depend on Boost. Short of that, we should make it an optional
download that people can get only when they want to. Thanks,
> regards,
> Florian Link
>
--
Zhanyong
Zhanyong Wan (λx.x x)
2009/10/9 Florian Link <flori...@gmail.com>
>
> Hi,
>
> I ported the files to boost thread, but now I noticed a problem.
> I thought that the multithreading support would mean that I could use
> ASSERT_* and EXPECT_* from other threads and
> the result would be added to the test function that was running in the
> main thread at that time.
This understanding is correct.
> But as far as I can see now, the thread support in google test means
> that you can run different tests in different threads
> (which is actually not what I need).
No, gtest only runs the tests in the main thread. A test method may
start new threads, and any assertion failure in such threads will be
attributed to the current test.
> Or did I misunderstand something?
> What would be needed to associate another thread with the same
> reporter of a running TestImpl, so that ASSERT_ etc. are part of that
> test?
I think your port of thread utilities probably has a different
semantics than what gtest assumes. I wrote this in issue 148:
"ThreadLocal is supposed to be initialized in the main thread only.
Its get() method
will return a different value for each thread that calls it. The
value is created
on-demand. If you call get() twice in the same thread, you get the
same value. If
you call get() from different threads, you get different values.
We use gtest in many multi-threaded tests where we have a full
implementation of
ThreadLocal, and it works fine."
Can this explain it? Thanks,
>
> regards,
> Florian
>
>
> 2009/10/8 Zhanyong Wan (λx.x x) <w...@google.com>:
--
Zhanyong
florianlink
the problem lies in the code below. The question is what
GetTestPartResultReporterForCurrentThread
should return. It effectively calls a
per_thread_test_part_result_reporter_.get(), which is declared as
follows:
internal::ThreadLocal<TestPartResultReporterInterface*>
per_thread_test_part_result_reporter_;
I found no code in google test that will call set() on
per_thread_test_part_result_reporter_ except for the main thread.
Now the question is: What should ThreadLocal<T*>::get() return in
another thread? It could return a) NULL, which will crash the code
below. Or it could b) return a new T(), in our case a new
TestPartResultReporterInterface().
I think both would be wrong, since I think the asserts from the
different thread should be added to the result reporter of the main
thread unit test! Thus, it is wrong to use ThreadLocalStorage for the
reporter. It is fine to use the mutex lock in the code below to avoid
races, but the reporter should be shared across threads.
Below the code that I think is wrong:
void UnitTest::AddTestPartResult(TestPartResultType result_type,
const char* file_name,
int line_number,
const internal::String& message,
const internal::String&
os_stack_trace) {
...
const TestPartResult result =
TestPartResult(result_type, file_name, line_number,
msg.GetString().c_str());
impl_->GetTestPartResultReporterForCurrentThread()->
ReportTestPartResult(result);
...
which should become something like:
const TestPartResult result =
TestPartResult(result_type, file_name, line_number,
msg.GetString().c_str());
impl_->GetGlobalTestPartResultReporter()->
ReportTestPartResult(result);
Apart from that, for gtest_trace_stack() using the ThreadLocal makes
perfect sense, since it will collect the scope traces on a per thread
basis.
Can some of the gtest developers comment on this, please?
regards,
Florian
On Oct 9, 6:38 pm, Zhanyong Wan (λx.x x) <w...@google.com> wrote:
> (adding back the gtest mailing list as I'm not the only one working on gtest)
>
Zhanyong Wan (λx.x x)
>
> Hi,
>
> the problem lies in the code below. The question is what
> GetTestPartResultReporterForCurrentThread
> should return. It effectively calls a
> per_thread_test_part_result_reporter_.get(), which is declared as
> follows:
>
> internal::ThreadLocal<TestPartResultReporterInterface*>
> per_thread_test_part_result_reporter_;
>
> I found no code in google test that will call set() on
> per_thread_test_part_result_reporter_ except for the main thread.
> Now the question is: What should ThreadLocal<T*>::get() return in
> another thread? It could return a) NULL, which will crash the code
> below. Or it could b) return a new T(), in our case a new
> TestPartResultReporterInterface().
It returns a pre-set value, which is set on gtest.cc:3744:
per_thread_test_part_result_reporter_(
&default_per_thread_test_part_result_reporter_),
So normally each thread will get the same default per-thread test part
result reporter. And thus test failures on all threads will be
attributed to the current test.
> I think both would be wrong, since I think the asserts from the
> different thread should be added to the result reporter of the main
> thread unit test! Thus, it is wrong to use ThreadLocalStorage for the
> reporter. It is fine to use the mutex lock in the code below to avoid
> races, but the reporter should be shared across threads.
It's neither.
> Below the code that I think is wrong:
>
> void UnitTest::AddTestPartResult(TestPartResultType result_type,
> const char* file_name,
> int line_number,
> const internal::String& message,
> const internal::String&
> os_stack_trace) {
> ...
> const TestPartResult result =
> TestPartResult(result_type, file_name, line_number,
> msg.GetString().c_str());
> impl_->GetTestPartResultReporterForCurrentThread()->
> ReportTestPartResult(result);
> ...
This is correct. The flexibility is needed for implementing macros in
gtest-spi.h.
--
Zhanyong
florianlink
thread, when the test is running, so the other threads will just get a
NULL value.
Or do I misunderstand the intention of ThreadLocal (which is not
documented at all...)? Is it supposed to return the value from the
main thread when it does not have
an own value set?
On Oct 9, 10:42 pm, Zhanyong Wan (λx.x x) <w...@google.com> wrote:
Zhanyong Wan (λx.x x)
>
> But the constructor of UnitTestImpl will only be called in the main
> thread, when the test is running, so the other threads will just get a
> NULL value.
> Or do I misunderstand the intention of ThreadLocal (which is not
> documented at all...)? Is it supposed to return the value from the
> main thread when it does not have
> an own value set?
Sorry for the lack of documentation.
The ctor of ThreadLocal<T> takes a default value (let's call it x).
get() returns a different *copy* for each thread in which it is called
(note that it returns a reference). These copies will all have an
initial value of x.
Example:
// Doesn't matter in which thread this is run:
ThreadLocal<int> x(42); // 42 is the default value.
...
// Called in thread #1:
const int& x1 = x.get(); // x1 has value 42.
// Called in thread #1 too:
const int& x1_too = x.get();
// x1_too references the same int object as x1 does.
...
// Called in thread #2:
const int& x2 = x.get(); // x2 has value 42.
// x2 and x1 reference different int objects.
So ThreadLocal<T> will give you one T object for each thread, which
you can read via get(). To set the initial value of these objects,
pass the value to the ctor of ThreadLocal<T>.
Hth,
--
Zhanyong
florianlink
I still had some trouble because internal::Vector does not allow
copying via assignment/copy constructor but it is used with
ThreadLocal,
so the generic ThreadLocal implementation would choke on the
assignment of the default value.
I worked around this by specializing for Vector<T>, there might be a
better solution to that.
Here you go:
#include <boost/thread/recursive_mutex.hpp>
#include <boost/thread/tss.hpp>
namespace testing {
namespace internal {
class Mutex {
public:
Mutex() {}
explicit Mutex(int /*unused*/) {}
enum { NO_CONSTRUCTOR_NEEDED_FOR_STATIC_MUTEX = 0 };
protected:
friend class GTestMutexLock;
boost::recursive_mutex _mutex;
};
// We cannot call it MutexLock directly as the ctor declaration would
// conflict with a macro named MutexLock, which is defined on some
// platforms. Hence the typedef trick below.
class GTestMutexLock {
public:
explicit GTestMutexLock(Mutex* mutex) {
_mutex = mutex;
_mutex->_mutex.lock();
}
~GTestMutexLock() {
_mutex->_mutex.unlock();
}
private:
Mutex* _mutex;
};
typedef GTestMutexLock MutexLock;
template <typename T>
class ThreadLocal {
public:
ThreadLocal() : defaultValue_() {
// defaultValue_() is important to initialize Ts of POD type (e.g.
T* or int) to 0
}
explicit ThreadLocal(const T& value): defaultValue_(value) {
// we do not store the value thread-local until someone calls
pointer() or get()
}
T* pointer() { return pointerWithDefaultCopy(); }
const T* pointer() const { return pointerWithDefaultCopy(); }
const T& get() const { return *pointerWithDefaultCopy(); }
void set(const T& value) { (*pointerWithDefaultCopy()) = value; }
private:
T* pointerWithDefaultCopy() const {
// if nothing is stored yet, create a copy of the default value
T* value = value_.get();
if (value == NULL) {
value = new T(defaultValue_);
value_.reset(value);
}
return value;
}
mutable boost::thread_specific_ptr<T> value_;
mutable T defaultValue_;
};
template <typename T> class Vector;
// partial specialization for Vector, since it does not
// allow copying with assignment/copy constructor
template <typename T>
class ThreadLocal<Vector<T> > {
public:
ThreadLocal() {
}
Vector<T>* pointer() { return pointerWithDefault(); }
const Vector<T>* pointer() const { return pointerWithDefault(); }
const Vector<T>& get() const { return *pointerWithDefault(); }
private:
Vector<T>* pointerWithDefault() const {
// if nothing is stored yet, create new empty value
Vector<T>* value = value_.get();
if (value == NULL) {
value = new Vector<T>();
value_.reset(value);
}
return value;
}
mutable boost::thread_specific_ptr<Vector<T> > value_;
};
On Oct 10, 12:07 am, Zhanyong Wan (λx.x x) <w...@google.com> wrote:
mfazekas
I've uploaded a pthread implementation for review at:
http://codereview.appspot.com/129067/show
To make both default construcible and copy constructable classes work
with ThreadLocal, instead of the
'value = new T(defaultValue_);'
I'm doing:
'value = copyOrConstruct(defaultValue_)'
where the copyOrConstruct is a function pointer, and it's intialized
in the constructor, to either:
static T* copy(const T& default_) { new T(default_); }
or:
static T* construct(const T&) { new T(); }
This way either copy constructor or default constructor is required
but not both.
Regards,
Miklos
>
> read more »
Miklós Fazekas
Any feedback on the pthread code?Questions:
1. I'm throwing an exception when the pthread api returns an error, is it ok, or should i use GTEST_CHECK_ instead?
2. A lot of code in Mutex 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.)
Thanks,
Miklós
Vlad Losev
Sorry for the delay; I am looking at your patch now and will reply to the issue directly.
Hi,
Any feedback on the pthread code?
Questions:
1. I'm throwing an exception when the pthread api returns an error, is it ok, or should i use GTEST_CHECK_ instead?
GTEST_CHECK_. Since Google Test has to work with exceptions disabled, it should not throw.
2. A lot of code in Mutex 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.)
We also have gtest_stress_test which checks threading robustness of Google Test. Currently it is disabled in the open source version and I am working on enabling it. I'll let you know when it is done.
Thanks,
MiklósOn Sun, Oct 11, 2009 at 8:02 PM, mfazekas <fazeka...@gmail.com> wrote:
Hi,
I've uploaded a pthread implementation for review at:
http://codereview.appspot.com/129067/show
To make both default construcible and copy constructable classes work
with ThreadLocal, instead of the
'value = new T(defaultValue_);'
I'm doing:
'value = copyOrConstruct(defaultValue_)'
where the copyOrConstruct is a function pointer, and it's intialized
in the constructor, to either:
static T* copy(const T& default_) { new T(default_); }
or:
static T* construct(const T&) { new T(); }
This way either copy constructor or default constructor is required
but not both.
Regards,
Vlad