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

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

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

> From: Seija Kijin <doremylover123@xxxxxxxxx>
>
> 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@xxxxxxxxx>
> ---
>     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@xxxxxxxxx
>
> 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.
	 */
> +		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?