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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Dec 19, 2022, 18:40 UTC
Message-ID: <221219.86ili7xmd5.gmgdl@evledraar.gmail.com>
In-Reply-To: <pull.1406.git.git.1671474876207.gitgitgadget@gmail.com>

On Mon, Dec 19 2022, Rose via GitGitGadget wrote:

Show 46 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.
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>     win32: close handles of threads that have been joined
>     
>     After joining threads, the handle to the original thread should be
>     closed as it no longer needs to be open.
>     
>     Signed-off-by: Seija Kijin doremylover123@gmail.com
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v1
> Pull-Request: https://github.com/git/git/pull/1406
>
>  compat/win32/pthread.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 2e7eead42cb..de89667ef70 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -39,14 +39,20 @@ 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());
> +	case WAIT_OBJECT_0:
> +		if (value_ptr)
> +			*value_ptr = thread->arg;
> +		/* detach the thread once the join succeeds */
> +		CloseHandle(thread->handle);
> +		return 0;
> +	case WAIT_ABANDONED:
> +		/* either thread is not joinable or another thread is waiting on
> +		 * this, so we do not detatch */

See CodingGuidelines for how multi-line comments should look like.

	/*
	 * Like this
	 * Another line etc.
	 */

Show 5 quoted lines

> +		return EINVAL;
> +	default:
> +	case WAIT_FAILED:
> +		/* the function failed so we do not detach */
> +		return err_win_to_posix(GetLastError());

The post-image adhares to our CodingGuidelines better than the pre-image, but please split up such re-indentation into a "prep" change. Manually looking at this with "git show -w" shows the actual (and smaller) functional change.

You add a "case" for WAIT_FAILED", but keep "default".

I have no idea about this API, but a search turned up: https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobject

That seems to suggest that it only returns 4 possible values.

Rather than having the "default" case shouldn't we (and this is just a suggestion, and should be its own prep change in any case) do:

	switch (result) {
	case WAIT_OBJECT_0:
		return ...;
	case WAIT_ABANDONED:
		return ...;
	case WAIT_TIMEOUT:
	case WAIT_FAILED:
		return ...;
	default:
		BUG("unhandled result %d", result);
	}

I.e. instead of keeping "default" you can just list "WAIT_TIMEOUT".

I don't know if that's OK with this API, it does say "If the function succeeds, the return value indicates, so maybe that "default" handles a lot more still?