Re: [PATCH v5] win32: close handles of threads that have been joined

From: Johannes Sixt <j6t@kdbg.org>
Date: Dec 20, 2022, 22:06 UTC
Message-ID: <4c4f7723-b1fa-846b-75ee-c37c61048ed8@kdbg.org>
In-Reply-To: <pull.1406.v5.git.git.1671571084753.gitgitgadget@gmail.com>

Am 20.12.22 um 22:18 schrieb Rose via GitGitGadget:

Show 8 quoted lines

> From: Seija Kijin <doremylover123@gmail.com>
> 
> After joining threads, the handle to the original thread
> should be closed as it no longer needs to be open.
> 
> Because this only needs to happen if the
> WaitForSingleObject fails, the function was
> rewritten to accommodate this change.

This sentence says that the handle must be closed only when WaitForSingleObject fails. But my understanding is that we must close it when the call is successful. In fact, that is what you implemented.

> The function is still POSIX compliant.
> 
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---

Show 31 quoted lines

> 
>  compat/win32/pthread.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 2e7eead42cb..7f8503b4b50 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -37,17 +37,15 @@ int pthread_create(pthread_t *thread, const void *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;
> -			return 0;
> -		case WAIT_ABANDONED:
> -			return EINVAL;
> -		default:
> -			return err_win_to_posix(GetLastError());
> +	if (WaitForSingleObject(thread->handle, INFINITE) == WAIT_FAILED)
> +		return err_win_to_posix(GetLastError());
> +
> +	if (value_ptr) {
> +		*value_ptr = thread->arg;
>  	}
> +
> +	CloseHandle(thread->handle);
> +	return 0;

Generally, such rewrites are not welcome if there is no obvious value in the new code structure. To my eyes, the original switch statement is much clearer than the new structure. In particular, the good case is when the result is WAIT_OBJECT_0. The switch statement clearly handles the case. The new code, however, loses the handling of a buggy caller, WAIT_ABANONED, and handles it like a success case.

What is wrong with just inserting a CloseHandle() call at the right spot in the original code and no other change?

>  }
>  
>  pthread_t pthread_self(void)

-- Hannes