diff mbox series

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

Message ID pull.1406.v5.git.git.1671571084753.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v5] win32: close handles of threads that have been joined | expand

Commit Message

Seija Kijin Dec. 20, 2022, 9:18 p.m. UTC 
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.

The function is still POSIX compliant.

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-v5
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v5
Pull-Request: https://github.com/git/git/pull/1406

Range-diff vs v4:

 1:  526ef7cc339 < -:  ----------- win32: close handles of threads that have been joined
 2:  2cb4d5c7007 ! 1:  94ed068d25b prep
     @@ Metadata
      Author: Seija Kijin <doremylover123@gmail.com>
      
       ## Commit message ##
     -    prep
     +    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.
     +
     +    Because this only needs to happen if the
     +    WaitForSingleObject fails, the function was
     +    rewritten to accommodate this change.
     +
     +    The function is still POSIX compliant.
      
          Signed-off-by: Seija Kijin <doremylover123@gmail.com>
      
       ## compat/win32/pthread.c ##
     -@@ compat/win32/pthread.c: static unsigned __stdcall win32_start_routine(void *arg)
     - }
     - 
     - int pthread_create(pthread_t *thread, const void *unused,
     --		   void *(*start_routine)(void*), void *arg)
     -+		   void *(*start_routine)(void *), void *arg)
     - {
     - 	thread->arg = arg;
     - 	thread->start_routine = start_routine;
     --	thread->handle = (HANDLE)
     --		_beginthreadex(NULL, 0, win32_start_routine, thread, 0, NULL);
     -+	thread->handle = (HANDLE)_beginthreadex(NULL, 0, win32_start_routine,
     -+						thread, 0, NULL);
     +@@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused,
       
     - 	if (!thread->handle)
     - 		return errno;
     -@@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_ptr)
     + int win32_pthread_join(pthread_t *thread, void **value_ptr)
       {
     - 	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
     - 	switch (result) {
     +-	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
     +-	switch (result) {
      -		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 do not detatch */
      -			return EINVAL;
      -		default:
     --			/* the function failed, so do not detach */
      -			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 do not detatch */
     -+		return EINVAL;
     -+	default:
     -+		/* the function failed, so do not detach */
     ++	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;
       }
       
     + pthread_t pthread_self(void)


 compat/win32/pthread.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)


base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Comments

Johannes Sixt Dec. 20, 2022, 10:06 p.m. UTC | #1 
Am 20.12.22 um 22:18 schrieb Rose via GitGitGadget:
> 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>
> ---

> 
>  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
diff mbox series

Patch

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;
 }
 
 pthread_t pthread_self(void)