Hi Tor,
On 7/23/10 4:33 AM, Tor Didriksen wrote:
> #At file:///export/home/didrik/repo/next-mr-bf-bug55434/ based on
> revid:jonathan.perkin@stripped
>
> 3319 Tor Didriksen 2010-07-23
> Bug #55434 multithreaded gunit tests fail on windows
Looks good, approved. Some minor suggestions below.
> On windows we need an open handle to the thread in order to join it.
> @ unittest/gunit/thread_utils.cc
> On windows: keep an open handle to a thread in order to join it safely.
>
> Use mysql_xxx() wrappers for pthread_xxx() functions.
> @ unittest/gunit/thread_utils.h
> Use mysql_mutex_t/mysql_cond_t rather than pthread_mutex_t/pthread_cond_t
Strictly speaking, you don't need to. But.. no problem.
>
> modified:
> unittest/gunit/thread_utils.cc
> unittest/gunit/thread_utils.h
> === modified file 'unittest/gunit/thread_utils.cc'
> --- a/unittest/gunit/thread_utils.cc 2010-05-21 13:02:27 +0000
> +++ b/unittest/gunit/thread_utils.cc 2010-07-23 07:33:41 +0000
> @@ -15,14 +15,41 @@
>
> #include<gtest/gtest.h>
> #include "thread_utils.h"
> +#include "mysql/psi/mysql_thread.h"
>
> namespace thread {
>
> +/*
> + On windows we need an open handle to the thread in order to join it.
> + The three instances of Notification are used as follows:
> + - The main thread starts sub-thread, waits for m_thread_started.
> + - The main thread picks up the thread id, and does OpenThread to get a handle.
> + - The main thread tells the sub-thread that it can continue
> + (notifies m_thread_continue)
> + - The main thread waits until the sub-thread is actually running
> + (waits for m_thread_running) before destroying all objects.
> +
> + All this is to work around a bug in pthread_create() in mysys/my_winthread,
> + which closes the thread handle (it has nowhere to store it).
> + When we later call pthread_join() we try to re-open a handle.
> + This will fail if the thread has already finished, and then the join will fail.
> + */
> +class Thread_start_arg
> +{
> +public:
> + Thread_start_arg(Thread *thread) : m_thread(thread) {}
> +
> + Notification m_thread_started;
> + Notification m_thread_continue;
> + Notification m_thread_running;
> + Thread *m_thread;
> +};
> +
> namespace {
> void *thread_start_routine(void *arg)
> {
> - Thread *thread= (Thread*) arg;
> - Thread::run_wrapper(thread);
> + Thread_start_arg *start_arg= (Thread_start_arg*) arg;
> + Thread::run_wrapper(start_arg);
> return NULL;
> }
>
> @@ -38,56 +65,93 @@ void assert_false(int arg, int line)
>
> Thread::~Thread()
> {
> +#ifdef __WIN__
> + if (m_thread_handle != 0)
0 -> NULL.
> + CloseHandle(m_thread_handle);
> +#endif
> }
>
>
> int Thread::start()
> {
> - return pthread_create(&m_thread_id, NULL, thread_start_routine, this);
> + Thread_start_arg start_arg(this);
> + const int retval=
> + pthread_create(&m_thread_id, NULL, thread_start_routine,&start_arg);
The below should only be done if pthread_create succeeded.
> + start_arg.m_thread_started.wait_for_notification();
> +#ifdef __WIN__
> + m_thread_handle= OpenThread(SYNCHRONIZE, FALSE, m_thread_id);
> +#endif
> + start_arg.m_thread_continue.notify();
> + start_arg.m_thread_running.wait_for_notification();
> + return retval;
> }
>
>
> void Thread::join()
> {
I suggest to ifdef the whole method. Makes it easier to read and step in
a debugger.
> - int failed= pthread_join(m_thread_id, NULL);
> - ASSERT_FALSE(failed);
> +#ifdef __WIN__
> + DWORD ret= WaitForSingleObject(m_thread_handle, INFINITE);
> + if (ret != WAIT_OBJECT_0)
> + {
> + DWORD lasterror= GetLastError();
> + ADD_FAILURE()
> +<< " could not join thread id "<< m_thread_id
> +<< " handle "<< m_thread_handle
> +<< " GetLastError: "<< lasterror
> + ;
Should we also close the handle here? It would make it more equivalent
to what pthread_join does.
> + }
> +#else
> + const int failed= pthread_join(m_thread_id, NULL);
> + if (failed)
> + {
> + ADD_FAILURE()
> +<< " could not join thread id "<< m_thread_id
> +<< " failed: "<< failed<< " errno: "<< errno
> + ;
> + }
> +#endif
> }
>
>
[..]
> - Mutex_lock(pthread_mutex_t *mutex);
> + Mutex_lock(mysql_mutex_t *mutex);
According to the style used in the file, you should drop the argument
name ;-)
> --- a/unittest/gunit/thread_utils.h 2010-03-19 11:46:14 +0000
> +++ b/unittest/gunit/thread_utils.h 2010-07-23 07:33:41 +0000
> @@ -21,6 +21,8 @@
>
> namespace thread {
>
> +class Thread_start_arg;
> +
> /*
> An abstract class for creating/running/joining threads.
> Thread::start() will create a new pthread, and execute the run() function.
> @@ -28,7 +30,11 @@ namespace thread {
> class Thread
> {
> public:
> - Thread() : m_thread_id(0) {}
> + Thread() : m_thread_id(0)
> +#ifdef __WIN__
> + , m_thread_handle(0)
0 -> NULL.
Otherwise looks good.
Regards,
Davi
