From: Erik Faye-Lund <kusmabite@gmail.com>
Date: Fri, 01 Apr 2011 19:33:46 GMT
Subject: Re: [PATCH 5/4] run-command: implement abort_async for pthreads
Message-ID: <AANLkTimk4cLDjj0xKgshm-Xqh7G2FeNA5Y-DXLmkEi3s@mail.gmail.com>
In-Reply-To: <AANLkTi=8+V3w-1uQgekjntM1qREGZYWSTE_ra1NbDxG8@mail.gmail.com>

On Fri, Apr 1, 2011 at 9:26 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Fri, Apr 1, 2011 at 7:27 PM, Johannes Sixt <j6t@kdbg.org> wrote:
>> On Freitag, 1. April 2011, Erik Faye-Lund wrote:
>>> On Fri, Apr 1, 2011 at 11:41 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>>> > On Thu, Mar 31, 2011 at 8:45 PM, Jeff King <peff@peff.net> wrote:
>>> >>        kill(async->pid, 15);
>>> >>  #else
>>> >> -       /* no clue */
>>> >> +       pthread_cancel(async->tid);
>>> >
>>> > My worry about terminating a thread that's currently holding a mutex
>>> > (implicitly through the CRT?) still applies though...
>>>
>>> OK, I've read up on thread-cancellation, and this code seems correct.
>>> pthread_cancel doesn't kill the thread right away, it just signals a
>>> cancellation-event, which is checked for at certain
>>> cancellation-points. A lot of the CRT functions are defined as
>>> cancellation points, so it'll be a matter for us Win32-guys to
>>> implement pthread_testcancel() and inject that into the
>>> function-wrappers of the CRT functions that are marked as
>>> cancellation-points.
>>
>> That's not going to happen. We cannot implement pthread_cancel() on Windows
>> because it would have to be able to interrupt blocking system calls.
>> (TerminateThread() is a no-no, given all the caveats about leaking system
>> resources that are mentioned in the manual.)
>>
>
> Did you read my suggestion? I was talking about implementing
> cancellation-points, just like on other platforms. This should not
> lead to TerminateThread, but instead a conditional ExitThread from the
> thread in question.
>
> Something like this (I've only added a cancellation-point at close,
> just to illustrate what I mean):
>
>
> diff --git a/compat/mingw.c b/compat/mingw.c
> index 878b1de..253be14 100644
> --- a/compat/mingw.c
> +++ b/compat/mingw.c
> @@ -302,6 +302,14 @@ int mingw_open (const char *filename, int oflags, ...)
>        return fd;
>  }
>
> +#undef close
> +int mingw_close(int fd)
> +{
> +       int ret = close(fd);
> +       pthread_testcancel();
> +       return ret;
> +}
> +
>  #undef write
>  ssize_t mingw_write(int fd, const void *buf, size_t count)
>  {
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 62eccd3..3e904c8 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -175,6 +175,9 @@ int mingw_rmdir(const char *path);
>  int mingw_open (const char *filename, int oflags, ...);
>  #define open mingw_open
>
> +int mingw_close(int fd);
> +#define close mingw_close
> +
>  ssize_t mingw_write(int fd, const void *buf, size_t count);
>  #define write mingw_write
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 010e875..47d620b 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -18,6 +18,7 @@ static unsigned __stdcall win32_start_routine(void *arg)
>        pthread_t *thread = arg;
>        thread->tid = GetCurrentThreadId();
>        thread->arg = thread->start_routine(thread->arg);
> +       CloseHandle(thread->cancel_event);
>        return 0;
>  }
>
> @@ -26,6 +27,9 @@ int pthread_create(pthread_t *thread, const void *unused,
>  {
>        thread->arg = arg;
>        thread->start_routine = start_routine;
> +       thread->cancel_event = CreateEvent(NULL, FALSE, FALSE, NULL);
> +       if (!thread->cancel_event)
> +               die("failed to create event");
>        thread->handle = (HANDLE)
>                _beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
>
> diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
> index 2e20548..7fce39d 100644
> --- a/compat/win32/pthread.h
> +++ b/compat/win32/pthread.h
> @@ -56,6 +56,7 @@ extern int pthread_cond_broadcast(pthread_cond_t *cond);
>  */
>  typedef struct {
>        HANDLE handle;
> +       HANDLE cancel_event;
>        void *(*start_routine)(void*);
>        void *arg;
>        DWORD tid;
> @@ -96,4 +97,13 @@ static inline void *pthread_getspecific(pthread_key_t key)
>        return TlsGetValue(key);
>  }
>
> +#define pthread_cancel(a) SetEvent(a.cancel_event)
> +
> +static inline void pthread_testcancel(void)
> +{
> +       pthread_t thread = pthread_self();
> +       if (WaitForSingleObject(thread.cancel_event, 0))

That should be:
-	if (WaitForSingleObject(thread.cancel_event, 0))
+	if (WaitForSingleObject(thread.cancel_event, 0) == WAIT_OBJECT_0)
...Of course.