From: Erik Faye-Lund <kusmabite@gmail.com>
To: Johannes Sixt <j6t@kdbg.org>
Cc: Jeff King <peff@peff.net>, git@vger.kernel.org
Subject: Re: [PATCH 5/4] run-command: implement abort_async for pthreads
Date: Fri, 1 Apr 2011 21:33:46 +0200 [thread overview]
Message-ID: <AANLkTimk4cLDjj0xKgshm-Xqh7G2FeNA5Y-DXLmkEi3s@mail.gmail.com> (raw)
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.
next prev parent reply other threads:[~2011-04-01 19:34 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-03-31 18:42 [PATCH 0/4] fix hang in git push when pack-objects fails Jeff King
2011-03-31 18:43 ` [PATCH 1/4] teach wait_or_whine a "quiet" mode Jeff King
2011-03-31 20:56 ` Johannes Sixt
2011-04-01 1:35 ` Jeff King
2011-03-31 18:44 ` [PATCH 2/4] finish_async: be quiet when waiting for async process Jeff King
2011-03-31 18:44 ` [PATCH 3/4] run-command: allow aborting async code prematurely Jeff King
2011-04-01 9:36 ` Erik Faye-Lund
2011-04-01 13:59 ` Jeff King
2011-03-31 18:44 ` [PATCH 4/4] send-pack: abort sideband demuxer on pack-objects error Jeff King
2011-04-13 19:53 ` Johannes Sixt
2011-04-14 13:54 ` Jeff King
2011-04-14 19:36 ` Johannes Sixt
2011-04-14 20:21 ` Jeff King
2011-04-14 20:43 ` Johannes Sixt
2011-04-14 20:51 ` Jeff King
2011-04-14 21:05 ` Johannes Sixt
2011-04-14 21:21 ` Junio C Hamano
2011-04-24 20:42 ` [PATCH/RFC 1/2] send-pack --stateless-rpc: properly close the outgoing channel Johannes Sixt
2011-04-24 20:49 ` [PATCH 2/2] send-pack: avoid deadlock when pack-object dies early Johannes Sixt
2011-04-25 16:50 ` Jeff King
2011-04-25 17:41 ` Johannes Sixt
2011-04-25 17:51 ` Junio C Hamano
2011-04-25 21:04 ` [PATCH v2] " Johannes Sixt
2011-04-26 8:23 ` Jeff King
2011-04-25 16:40 ` [PATCH/RFC 1/2] send-pack --stateless-rpc: properly close the outgoing channel Jeff King
2011-03-31 18:45 ` [PATCH 5/4] run-command: implement abort_async for pthreads Jeff King
2011-04-01 9:41 ` Erik Faye-Lund
2011-04-01 10:15 ` Erik Faye-Lund
2011-04-01 17:27 ` Johannes Sixt
2011-04-01 17:38 ` Jeff King
2011-04-01 19:26 ` Erik Faye-Lund
2011-04-01 19:33 ` Erik Faye-Lund [this message]
2011-04-01 19:42 ` Johannes Sixt
2011-04-01 19:57 ` Erik Faye-Lund
2011-04-01 20:05 ` Jeff King
2011-04-01 20:13 ` Erik Faye-Lund
2011-04-01 20:17 ` Jeff King
2011-04-01 20:18 ` Jeff King
2011-04-01 20:34 ` Erik Faye-Lund
2011-04-01 20:36 ` Johannes Sixt
2011-04-01 20:41 ` Erik Faye-Lund
2011-04-01 20:18 ` Johannes Sixt
2011-04-01 20:31 ` Erik Faye-Lund
2011-04-01 21:16 ` Jeff King
2011-04-02 12:27 ` Erik Faye-Lund
2011-04-01 14:00 ` Jeff King
2011-03-31 20:45 ` [PATCH 0/4] fix hang in git push when pack-objects fails Johannes Sixt
2011-04-01 1:34 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=AANLkTimk4cLDjj0xKgshm-Xqh7G2FeNA5Y-DXLmkEi3s@mail.gmail.com \
--to=kusmabite@gmail.com \
--cc=git@vger.kernel.org \
--cc=j6t@kdbg.org \
--cc=peff@peff.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).