List:Commits« Previous MessageNext Message »
From:Davi Arnaut Date:July 23 2010 11:12am
Subject:Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3319)
Bug#55434
View as plain text  
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
Thread
bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3319) Bug#55434Tor Didriksen23 Jul
  • Re: bzr commit into mysql-next-mr-bugfixing branch (tor.didriksen:3319)Bug#55434Davi Arnaut23 Jul