Re: [PATCH v2] win32: remove handling for impossible cases in win32_pthread_join

[Junio C Hamano <gitster@pobox.com>]

"AZero13 via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Greg Funni <gfunni234@gmail.com>

Looking much better, but there still are some puzzlement left.

> WAIT_FAILED is the only real possible error here.
>
> WAIT_TIMEOUT would not be returned as the INFINITE
> is given to the call.

OK.

> WAIT_ABANDONED would be returned if the handle
> pointed to a mutex object that was not released
> by the thread that owned the mutex object before
> the owning thread terminated.

... and we know the handle we are passing to WaitForSingleObject()
is a Thread, not a Mutex, so this error condition is irrelevant?

OK.

> WAIT_IO_COMPLETION would not be returned because
> we pass FALSE so the wait is not alertable.

FALSE where?  Ah, this can be done with WaitForSingleObjectEx, but
WaitForSingleObject() is what we call here, so WAIT_IO_COMPLETION
won't be returned with or without FALSE.  Is there a reason why you
want to change the code to call Ex variant (the manual page tells us
to use it _if_ we want to enter an alertable wait state, and I am
assuming that we are not interested in doing so)?

In any case, among the four possible return values from
WaitForSingleObject(), we know WAIT_ABANDONED and WAIT_TIMEOUT will
not be relevant for this code path.

Because WAIT_OBJECT_0 is the cryptic synonym for "Success!" for this
call, WAIT_FAILED is indeed the only possible error here, just like
you said at the beginning.

I think it is easier to understand for mere-mortal readers like me,
who are not familiar with Win32 API, if we explained this change
more like:

  Subject: [PATCH] win32: simplify win32_pthread_join() error handling

  Among the four possible result WaitForSingleObject() can return,
  WAIT_TIMEOUT and WAIT_ABANDONED are not relevant in this code
  path, because we do not ask for the call to time-out, and we do
  not pass a mutex object to the call (we are passing a thread
  object).

  Simplify the code to

  - return an error without closing the handle if the call failed
    (i.e., returns WAIT_FAILED that is not zero);

  - otherwise, WAIT_OBJECT_0 (which is 0) is returned to signal a
    success.  Do exactly what the original code did in this case.

What do you think?

> Signed-off-by: Greg Funni <gfunni234@gmail.com>
>  compat/win32/pthread.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)


> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 58980a529c..54c43b4146 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr UNUSED,
>  
>  int win32_pthread_join(pthread_t *thread, void **value_ptr)
>  {
> -	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
> -	switch (result) {
> -	case WAIT_OBJECT_0:
> -		if (value_ptr)
> -			*value_ptr = thread->arg;
> -		CloseHandle(thread->handle);
> -		return 0;
> -	case WAIT_ABANDONED:
> -		CloseHandle(thread->handle);
> -		return EINVAL;
> -	default:

And the above is what the patch simplifies away, which is great.

> -		/* the wait failed, so do not detach */

I think this comment is worth keeping (I am assuming "do not detach"
refers to the fact that CloseHandle(thread->handle) is not called in
the error case).

> +	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)
>  		return err_win_to_posix(GetLastError());

The change is based on out belief that WAIT_FAILED is the only
possible error from this call.  Even if our belief turns out to be
wrong, we would want to take the error code path, wouldn't we?  IOW,
I think the above should be more like

	if (WaitForSingleObject(thread->handle, INFINITE))
		/* the wait failed; do not detach */
		return err_win_to_posix(GetLastError());

i.e., if we get an error, report the error to the caller, regardless
of what kind of an error it is, even though we expect it to be a
WAIT_FAILED.  And the case this if() condition does not catch is a
successful wait (i.e., WAIT_OBJECT_0), which is handled ...

> +	if (value_ptr)
> +		*value_ptr = thread->arg;
> +
> +	CloseHandle(thread->handle);
> +	return 0;

... exactly as before.

>  }
>  
>  pthread_t pthread_self(void)
>
> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed

Looking good.  Thanks.