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

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 18, 2025, 17:02 UTC
Message-ID: <xmqq4iqrff4j.fsf@gitster.g>
In-Reply-To: <pull.2102.v2.git.git.1763480720264.gitgitgadget@gmail.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(-)

Show 19 quoted lines

> 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 ...

Show 5 quoted lines

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

... exactly as before.

Show 5 quoted lines

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

Looking good.  Thanks.