threads / patch / 58983

patchwin32: close handles of threads that have been joined

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

## tl;dr

41 messages between Dec 19, 2022 and Jan 6, 2023. Diffs are folded; open one to read it.

replies: 40people: 6as markdown or json

Rose via GitGitGadget· Dec 19, 2022, 18:34 UTC · lore
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(-)
Show changes to compat/win32/pthread.c +14 −8
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 */
+		return EINVAL;
+	default:
+	case WAIT_FAILED:
+		/* the function failed so we do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget
Ævar Arnfjörð Bjarmason· Dec 19, 2022, 18:40 UTC · re: Rose via GitGitGadget · lore

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

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?

Johannes Sixt· Dec 20, 2022, 07:53 UTC · re: Rose via GitGitGadget · lore

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

Am 19.12.22 um 19:34 schrieb Rose via GitGitGadget:
Show 7 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>
> ---
Show 25 quoted lines
>  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;

This is a good change. It is a severe omission that the handle was not closed. (But I still have to test the patch.)

> +	case WAIT_ABANDONED:
> +		/* either thread is not joinable or another thread is waiting on
> +		 * this, so we do not detatch */
> +		return EINVAL;

I don't know which cases this mental note wants to help. Assuming that the [win232_]pthread_ API is used correctly, this error cannot happen (WAIT_ABANDONED can only happen when WaitForSingleObject is called on a mutex object).

Show 5 quoted lines
> +	default:
> +	case WAIT_FAILED:
> +		/* the function failed so we do not detach */
> +		return err_win_to_posix(GetLastError());
>  	}
Good.
-- Hannes
Rose via GitGitGadget· Dec 20, 2022, 18:57 UTC · re: Rose via GitGitGadget · lore

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

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-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v2
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v1:
 1:  e53b274ab4b ! 1:  8b20c4149be win32: close handles of threads that have been joined
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +		return 0;
      +	case WAIT_ABANDONED:
      +		/* either thread is not joinable or another thread is waiting on
     -+		 * this, so we do not detatch */
     ++		 * this, so do not detatch */
      +		return EINVAL;
     -+	default:
      +	case WAIT_FAILED:
     ++	default:
      +		/* the function failed so we do not detach */
      +		return err_win_to_posix(GetLastError());
       	}
 compat/win32/pthread.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
Show changes to compat/win32/pthread.c +14 −8
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..0d62c45ecc2 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 do not detatch */
+		return EINVAL;
+	case WAIT_FAILED:
+	default:
+		/* the function failed so we do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget
Rose via GitGitGadget· Dec 20, 2022, 19:00 UTC · re: Rose via GitGitGadget · lore

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

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-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v3
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v2:
 1:  8b20c4149be ! 1:  2049576b193 win32: close handles of threads that have been joined
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +		return EINVAL;
      +	case WAIT_FAILED:
      +	default:
     -+		/* the function failed so we do not detach */
     ++		/* the function failed, so do not detach */
      +		return err_win_to_posix(GetLastError());
       	}
       }
 compat/win32/pthread.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)
Show changes to compat/win32/pthread.c +14 −8
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..306834a7153 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 do not detatch */
+		return EINVAL;
+	case WAIT_FAILED:
+	default:
+		/* the function failed, so do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget
Rose via GitGitGadget· Dec 20, 2022, 20:03 UTC · re: Rose via GitGitGadget · lore

[PATCH v4 0/2] 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
Seija Kijin (2):
  win32: close handles of threads that have been joined
  prep
 compat/win32/pthread.c | 27 ++++++++++++++++-----------
 1 file changed, 16 insertions(+), 11 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v4
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v3:
 -:  ----------- > 1:  526ef7cc339 win32: close handles of threads that have been joined
 1:  2049576b193 ! 2:  2cb4d5c7007 win32: close handles of threads that have been joined
     @@ Metadata
      Author: Seija Kijin <doremylover123@gmail.com>
      
       ## Commit message ##
     -    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.
     +    prep
      
          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);
     + 
     + 	if (!thread->handle)
     + 		return errno;
      @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_ptr)
       {
       	DWORD result = WaitForSingleObject(thread->handle, INFINITE);
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      -		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)
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +		CloseHandle(thread->handle);
      +		return 0;
      +	case WAIT_ABANDONED:
     -+		/* either thread is not joinable or another thread is waiting on
     -+		 * this, so do not detatch */
     ++		/* either thread is not joinable or another thread is
     ++		 * waiting on this, so do not detatch */
      +		return EINVAL;
     -+	case WAIT_FAILED:
      +	default:
      +		/* the function failed, so do not detach */
      +		return err_win_to_posix(GetLastError());
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 20, 2022, 20:04 UTC · re: Rose via GitGitGadget · lore

[PATCH v4 1/2] win32: close handles of threads that have been joined

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>
---
 compat/win32/pthread.c | 5 +++++
 1 file changed, 5 insertions(+)
Show changes to compat/win32/pthread.c +5 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..89e89c3fe00 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,15 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 		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());
 	}
 }
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 20, 2022, 20:04 UTC · re: Rose via GitGitGadget · lore

[PATCH v4 2/2] prep

From: Seija Kijin <doremylover123@gmail.com>
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)
Show changes to compat/win32/pthread.c +16 −16
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 89e89c3fe00..0831ffbc3ae 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -39,19 +39,19 @@ 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;
-			/* 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 */
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Rose via GitGitGadget· Dec 20, 2022, 21:18 UTC · re: Rose via GitGitGadget · lore

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

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(-)
Show changes to compat/win32/pthread.c +8 −10
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)

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget
Johannes Sixt· Dec 20, 2022, 22:06 UTC · re: Rose via GitGitGadget · lore

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

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
Rose via GitGitGadget· Dec 21, 2022, 04:35 UTC · re: Rose via GitGitGadget · lore

[PATCH v6 0/2] 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
Seija Kijin (2):
  win32: close handles of threads that have been joined
  prep
 compat/win32/pthread.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v6
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v6
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v5:
 1:  94ed068d25b ! 1:  0b5afdb93db win32: close handles of threads that have been joined
     @@ Commit message
          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.
     +    This change makes the function POSIX compliant.
      
          Signed-off-by: Seija Kijin <doremylover123@gmail.com>
      
     @@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused
      -			return EINVAL;
      -		default:
      -			return err_win_to_posix(GetLastError());
     -+	if (WaitForSingleObject(thread->handle, INFINITE) == WAIT_FAILED)
     ++	switch (WaitForSingleObject(thread->handle, INFINITE)) {
     ++	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:
     ++		/* the function failed, so do not detach */
      +		return err_win_to_posix(GetLastError());
     -+
     -+	if (value_ptr) {
     -+		*value_ptr = thread->arg;
       	}
     -+
     -+	CloseHandle(thread->handle);
     -+	return 0;
       }
       
     - pthread_t pthread_self(void)
 -:  ----------- > 2:  5b35362e5d2 prep
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 21, 2022, 04:35 UTC · re: Rose via GitGitGadget · lore

[PATCH v6 1/2] win32: close handles of threads that have been joined

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.

This change makes the function POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)
Show changes to compat/win32/pthread.c +12 −10
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..cf94b4491f9 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -37,16 +37,18 @@ 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());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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:
+		/* the function failed, so do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 21, 2022, 04:35 UTC · re: Rose via GitGitGadget · lore

[PATCH v6 2/2] prep

From: Seija Kijin <doremylover123@gmail.com>
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to compat/win32/pthread.c +3 −3
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index cf94b4491f9..fc88ce20e2b 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
-- 
gitgitgadget
Rose via GitGitGadget· Dec 21, 2022, 04:46 UTC · re: Rose via GitGitGadget · lore

[PATCH v7 0/2] 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
Seija Kijin (2):
  win32: close handles of threads that have been joined
  prep
 compat/win32/pthread.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v7
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v6:
 -:  ----------- > 1:  b40287508df win32: close handles of threads that have been joined
 1:  0b5afdb93db ! 2:  f780ed525eb win32: close handles of threads that have been joined
     @@ Metadata
      Author: Seija Kijin <doremylover123@gmail.com>
      
       ## Commit message ##
     -    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.
     -
     -    This change makes the function POSIX compliant.
     +    prep
      
          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);
     + 
     + 	if (!thread->handle)
     + 		return errno;
      @@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused,
       
       int win32_pthread_join(pthread_t *thread, void **value_ptr)
     @@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused
      -		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:
     +-			/* the function failed, so do not detach */
      -			return err_win_to_posix(GetLastError());
      +	switch (WaitForSingleObject(thread->handle, INFINITE)) {
      +	case WAIT_OBJECT_0:
 2:  5b35362e5d2 < -:  ----------- prep
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 21, 2022, 04:46 UTC · re: Rose via GitGitGadget · lore

[PATCH v7 2/2] prep

From: Seija Kijin <doremylover123@gmail.com>
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
Show changes to compat/win32/pthread.c +15 −16
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 21c705778b6..fc88ce20e2b 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -37,19 +37,18 @@ 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;
-			CloseHandle(thread->handle);
-			return 0;
-		case WAIT_ABANDONED:
-			CloseHandle(thread->handle);
-			return EINVAL;
-		default:
-			/* the function failed, so do not detach */
-			return err_win_to_posix(GetLastError());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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:
+		/* the function failed, so do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 21, 2022, 04:46 UTC · re: Rose via GitGitGadget · lore

[PATCH v7 1/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..21c705778b6 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 		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:
+			/* the function failed, so do not detach */
 			return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Johannes Sixt· Dec 22, 2022, 07:52 UTC · re: Seija Kijin via GitGitGadget · lore

Re: [PATCH v7 1/2] win32: close handles of threads that have been joined

Am 21.12.22 um 05:46 schrieb Seija Kijin via GitGitGadget:
Show 30 quoted lines
> From: Seija Kijin <doremylover123@gmail.com>
> 
> After the thread terminates, the handle to the
> original thread should be closed.
> 
> This change makes win32_pthread_join POSIX compliant.
> 
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>  compat/win32/pthread.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 2e7eead42cb..21c705778b6 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
>  		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:
> +			/* the function failed, so do not detach */
>  			return err_win_to_posix(GetLastError());
>  	}
>  }
This patch looks good. It passes the test suite on Windows. Hence:
Acked-by: Johannes Sixt <j6t@kdbg.org>

The follow-up patch in this thread, though, has an incomplete subject line and no motivation. It is not ready to be picked up.

-- Hannes
Rose via GitGitGadget· Dec 22, 2022, 16:01 UTC · re: Rose via GitGitGadget · lore

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

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join 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-v8
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v8
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v7:
 1:  b40287508df = 1:  70588032eb3 win32: close handles of threads that have been joined
 2:  f780ed525eb < -:  ----------- prep
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..21c705778b6 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 		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:
+			/* the function failed, so do not detach */
 			return err_win_to_posix(GetLastError());
 	}
 }

base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
-- 
gitgitgadget
Rose via GitGitGadget· Dec 22, 2022, 17:17 UTC · re: Rose via GitGitGadget · lore

[PATCH v9 0/2] 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
Seija Kijin (2):
  win32: close handles of threads that have been joined
  prep: reformat pthread.c to fit coding guidelines
 compat/win32/pthread.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v9
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v9
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v8:
 1:  70588032eb3 ! 1:  d9b1df22e03 win32: close handles of threads that have been joined
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +			CloseHandle(thread->handle);
       			return EINVAL;
       		default:
     -+			/* the function failed, so do not detach */
     ++			/* the wait failed, so do not detach */
       			return err_win_to_posix(GetLastError());
       	}
       }
 -:  ----------- > 2:  4c82a16a995 prep: reformat pthread.c to fit coding guidelines
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 22, 2022, 17:17 UTC · re: Rose via GitGitGadget · lore

[PATCH v9 2/2] prep: reformat pthread.c to fit coding guidelines

From: Seija Kijin <doremylover123@gmail.com>

The motivation for this change is that the post-image better fits the coding guidelines, especially since this file was changed.

Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)
Show changes to compat/win32/pthread.c +15 −16
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 81178ed93b7..83e088dff0a 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -37,19 +37,18 @@ 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;
-			CloseHandle(thread->handle);
-			return 0;
-		case WAIT_ABANDONED:
-			CloseHandle(thread->handle);
-			return EINVAL;
-		default:
-			/* the wait failed, so do not detach */
-			return err_win_to_posix(GetLastError());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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:
+		/* the wait failed, so do not detach */
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Junio C Hamano· Dec 23, 2022, 01:51 UTC · re: Seija Kijin via GitGitGadget · lore

Re: [PATCH v9 2/2] prep: reformat pthread.c to fit coding guidelines

"Seija Kijin via GitGitGadget" <gitgitgadget@gmail.com> writes:
> Subject: Re: [PATCH v9 2/2] prep: reformat pthread.c to fit coding guidelines

I am guessing that you meant to have "prep" stand for "preparation" (which by the way is not a good thing to have instead of <area>: prefix). Perhaps "windows-pthread:" or something?

In any case, this step should be done first, i.e. [1/2] of a two-patch series, as a preparation for the real change, I would think.

Seija Kijin via GitGitGadget· Dec 22, 2022, 17:17 UTC · re: Rose via GitGitGadget · lore

[PATCH v9 1/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..81178ed93b7 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 		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:
+			/* the wait failed, so do not detach */
 			return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Rose via GitGitGadget· Dec 23, 2022, 19:15 UTC · re: Rose via GitGitGadget · lore

[PATCH v10 0/2] 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
Seija Kijin (2):
  win32-pthread: prepare pthread.c for change by formatting
  win32: close handles of threads that have been joined
 compat/win32/pthread.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v10
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v10
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v9:
 2:  4c82a16a995 ! 1:  e0cc43efc6c prep: reformat pthread.c to fit coding guidelines
     @@ Metadata
      Author: Seija Kijin <doremylover123@gmail.com>
      
       ## Commit message ##
     -    prep: reformat pthread.c to fit coding guidelines
     +    win32-pthread: prepare pthread.c for change by formatting
      
     -    The motivation for this change is that
     -    the post-image better fits the coding
     -    guidelines, especially since this file
     -    was changed.
     +    File has been formatted to meet coding guidelines
      
          Signed-off-by: Seija Kijin <doremylover123@gmail.com>
      
     @@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused
      -		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:
     --			/* the wait failed, so do not detach */
      -			return err_win_to_posix(GetLastError());
      +	switch (WaitForSingleObject(thread->handle, INFINITE)) {
      +	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:
     -+		/* the wait failed, so do not detach */
      +		return err_win_to_posix(GetLastError());
       	}
       }
 1:  d9b1df22e03 ! 2:  d05d4c2e4b2 win32: close handles of threads that have been joined
     @@ Commit message
      
       ## compat/win32/pthread.c ##
      @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_ptr)
     - 		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:
     -+			/* the wait failed, so do not detach */
     - 			return err_win_to_posix(GetLastError());
     + 	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:
     ++		/* the wait failed, so do not detach */
     + 		return err_win_to_posix(GetLastError());
       	}
       }
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 23, 2022, 19:15 UTC · re: Rose via GitGitGadget · lore

[PATCH v10 1/2] win32-pthread: prepare pthread.c for change by formatting

From: Seija Kijin <doremylover123@gmail.com>
File has been formatted to meet coding guidelines
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
Show changes to compat/win32/pthread.c +12 −13
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..65e15a560d5 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -37,16 +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());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 23, 2022, 19:15 UTC · re: Rose via GitGitGadget · lore

[PATCH v10 2/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 65e15a560d5..83e088dff0a 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -41,10 +41,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 	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:
+		/* the wait failed, so do not detach */
 		return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Rose via GitGitGadget· Dec 23, 2022, 19:19 UTC · re: Rose via GitGitGadget · lore

[PATCH v11 0/2] 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
Seija Kijin (2):
  win32-pthread: prepare pthread.c for change by formatting
  win32: close handles of threads that have been joined
 compat/win32/pthread.c | 29 ++++++++++++++++-------------
 1 file changed, 16 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v11
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v11
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v10:
 1:  e0cc43efc6c = 1:  e0cc43efc6c win32-pthread: prepare pthread.c for change by formatting
 2:  d05d4c2e4b2 ! 2:  c5d2ddad166 win32: close handles of threads that have been joined
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +		CloseHandle(thread->handle);
       		return EINVAL;
       	default:
     +-		return err_win_to_posix(GetLastError());
      +		/* the wait failed, so do not detach */
     - 		return err_win_to_posix(GetLastError());
     ++		errno = err_win_to_posix(GetLastError());
     ++		return errno;
       	}
       }
     + 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 23, 2022, 19:19 UTC · re: Rose via GitGitGadget · lore

[PATCH v11 1/2] win32-pthread: prepare pthread.c for change by formatting

From: Seija Kijin <doremylover123@gmail.com>
File has been formatted to meet coding guidelines
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
Show changes to compat/win32/pthread.c +12 −13
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..65e15a560d5 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -37,16 +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());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 23, 2022, 19:19 UTC · re: Rose via GitGitGadget · lore

[PATCH v11 2/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to compat/win32/pthread.c +5 −1
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 65e15a560d5..f63f65c9dfe 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -41,11 +41,15 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 	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:
-		return err_win_to_posix(GetLastError());
+		/* the wait failed, so do not detach */
+		errno = err_win_to_posix(GetLastError());
+		return errno;
 	}
 }
 
-- 
gitgitgadget
René Scharfe· Dec 24, 2022, 07:50 UTC · re: Seija Kijin via GitGitGadget · lore

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

Am 23.12.22 um 20:19 schrieb Seija Kijin via GitGitGadget:
Show 30 quoted lines
> From: Seija Kijin <doremylover123@gmail.com>
>
> After the thread terminates, the handle to the
> original thread should be closed.
>
> This change makes win32_pthread_join POSIX compliant.
>
> Signed-off-by: Seija Kijin <doremylover123@gmail.com>
> ---
>  compat/win32/pthread.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 65e15a560d5..f63f65c9dfe 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -41,11 +41,15 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
>  	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:
> -		return err_win_to_posix(GetLastError());
> +		/* the wait failed, so do not detach */
> +		errno = err_win_to_posix(GetLastError());
> +		return errno;

Why do we need to set errno? That's the only difference to v10, but I didn't find an explanation. POSIX only mentions the return value: https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_join.html

>  	}
>  }
>
Rose via GitGitGadget· Dec 25, 2022, 01:40 UTC · re: Rose via GitGitGadget · lore

[PATCH v12 0/2] 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
Seija Kijin (2):
  win32-pthread: prepare pthread.c for change by formatting
  win32: close handles of threads that have been joined
 compat/win32/pthread.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)
base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v12
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v12
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v11:
 1:  e0cc43efc6c = 1:  e0cc43efc6c win32-pthread: prepare pthread.c for change by formatting
 2:  c5d2ddad166 ! 2:  fa5cbfa5e0c win32: close handles of threads that have been joined
     @@ compat/win32/pthread.c: int win32_pthread_join(pthread_t *thread, void **value_p
      +		CloseHandle(thread->handle);
       		return EINVAL;
       	default:
     --		return err_win_to_posix(GetLastError());
      +		/* the wait failed, so do not detach */
     -+		errno = err_win_to_posix(GetLastError());
     -+		return errno;
     + 		return err_win_to_posix(GetLastError());
       	}
       }
     - 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Dec 25, 2022, 01:40 UTC · re: Rose via GitGitGadget · lore

[PATCH v12 2/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 65e15a560d5..83e088dff0a 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -41,10 +41,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 	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:
+		/* the wait failed, so do not detach */
 		return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Junio C Hamano· Dec 26, 2022, 02:07 UTC · re: Seija Kijin via GitGitGadget · lore

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

"Seija Kijin via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 18 quoted lines
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 65e15a560d5..83e088dff0a 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -41,10 +41,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
>  	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:
> +		/* the wait failed, so do not detach */
>  		return err_win_to_posix(GetLastError());
>  	}
>  }
This matches what got an Ack from J6t in v7, so I'll queue with
Acked-by: added.  Thanks.
Seija Kijin via GitGitGadget· Dec 25, 2022, 01:40 UTC · re: Rose via GitGitGadget · lore

[PATCH v12 1/2] win32-pthread: prepare pthread.c for change by formatting

From: Seija Kijin <doremylover123@gmail.com>
File has been formatted to meet coding guidelines
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)
Show changes to compat/win32/pthread.c +12 −13
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..65e15a560d5 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -37,16 +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());
+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
+	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());
 	}
 }
 
-- 
gitgitgadget
Junio C Hamano· Dec 26, 2022, 01:32 UTC · re: Seija Kijin via GitGitGadget · lore

Re: [PATCH v12 1/2] win32-pthread: prepare pthread.c for change by formatting

"Seija Kijin via GitGitGadget" <gitgitgadget@gmail.com> writes:
>  int pthread_create(pthread_t *thread, const void *unused,
> -		   void *(*start_routine)(void*), void *arg)
> +		   void *(*start_routine)(void *), void *arg)
Good.
Show 7 quoted lines
>  {
>  	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);

I would say this is good either way---preimage may slightly be easier to read, but it is not worth a patch churn.

Show 21 quoted lines
> -	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());
> +	switch (WaitForSingleObject(thread->handle, INFINITE)) {
> +	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());
>  	}
>  }

Loss of a single-use variable is, strictly speaking, a "while at it" change that has nothing to do with "reformatting", but otherwise the result of the hunk looks good.

Will queue.
Thanks.

P.S. An address that is designed to bounce or sent to black hole appears on the CC: of the original e-mail message. Can you stop doing that (or is that what GGG does without user's control)?

  Rose <83477269+AtariDreams@users.noreply.github.com>
Rose via GitGitGadget· Jan 3, 2023, 16:20 UTC · re: Rose via GitGitGadget · lore

[PATCH v13 0/2] 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
Seija Kijin (2):
  win32: prepare pthread.c for change by formatting
  win32: close handles of threads that have been joined
 compat/win32/pthread.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
base-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v13
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v13
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v12:
 1:  e0cc43efc6c ! 1:  6f89b120641 win32-pthread: prepare pthread.c for change by formatting
     @@ Metadata
      Author: Seija Kijin <doremylover123@gmail.com>
      
       ## Commit message ##
     -    win32-pthread: prepare pthread.c for change by formatting
     +    win32: prepare pthread.c for change by formatting
      
     -    File has been formatted to meet coding guidelines
     +    File has been formatted to meet coding guidelines.
      
          Signed-off-by: Seija Kijin <doremylover123@gmail.com>
      
     @@ compat/win32/pthread.c: static unsigned __stdcall win32_start_routine(void *arg)
       
       	if (!thread->handle)
       		return errno;
     -@@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused,
     - 
     - int win32_pthread_join(pthread_t *thread, void **value_ptr)
     +@@ compat/win32/pthread.c: 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;
     @@ compat/win32/pthread.c: int pthread_create(pthread_t *thread, const void *unused
      -			return EINVAL;
      -		default:
      -			return err_win_to_posix(GetLastError());
     -+	switch (WaitForSingleObject(thread->handle, INFINITE)) {
      +	case WAIT_OBJECT_0:
      +		if (value_ptr)
      +			*value_ptr = thread->arg;
 2:  fa5cbfa5e0c = 2:  f126d6416d5 win32: close handles of threads that have been joined
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Jan 3, 2023, 16:20 UTC · re: Rose via GitGitGadget · lore

[PATCH v13 1/2] win32: prepare pthread.c for change by formatting

From: Seija Kijin <doremylover123@gmail.com>
File has been formatted to meet coding guidelines.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Show changes to compat/win32/pthread.c +11 −11
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..cf53bc61d82 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -39,14 +39,14 @@ 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;
+		return 0;
+	case WAIT_ABANDONED:
+		return EINVAL;
+	default:
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Jan 3, 2023, 16:20 UTC · re: Rose via GitGitGadget · lore

[PATCH v13 2/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index cf53bc61d82..85f8f7920ce 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 	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:
+		/* the wait failed, so do not detach */
 		return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Rose via GitGitGadget· Jan 5, 2023, 15:44 UTC · re: Rose via GitGitGadget · lore

[PATCH v14 0/2] 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
Seija Kijin (2):
  win32: prepare pthread.c for change by formatting
  win32: close handles of threads that have been joined
 compat/win32/pthread.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)
base-commit: 4dbebc36b0893f5094668ddea077d0e235560b16
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v14
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v14
Pull-Request: https://github.com/git/git/pull/1406
Range-diff vs v13:
 1:  6f89b120641 = 1:  a20eafb9541 win32: prepare pthread.c for change by formatting
 2:  f126d6416d5 = 2:  aa0319bc08e win32: close handles of threads that have been joined
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Jan 5, 2023, 15:44 UTC · re: Rose via GitGitGadget · lore

[PATCH v14 1/2] win32: prepare pthread.c for change by formatting

From: Seija Kijin <doremylover123@gmail.com>
File has been formatted to meet coding guidelines.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)
Show changes to compat/win32/pthread.c +11 −11
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 2e7eead42cb..cf53bc61d82 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -22,12 +22,12 @@ 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);
 
 	if (!thread->handle)
 		return errno;
@@ -39,14 +39,14 @@ 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;
+		return 0;
+	case WAIT_ABANDONED:
+		return EINVAL;
+	default:
+		return err_win_to_posix(GetLastError());
 	}
 }
 
-- 
gitgitgadget
Seija Kijin via GitGitGadget· Jan 5, 2023, 15:44 UTC · re: Rose via GitGitGadget · lore

[PATCH v14 2/2] win32: close handles of threads that have been joined

From: Seija Kijin <doremylover123@gmail.com>

After the thread terminates, the handle to the original thread should be closed.

This change makes win32_pthread_join POSIX compliant.
Signed-off-by: Seija Kijin <doremylover123@gmail.com>
---
 compat/win32/pthread.c | 3 +++
 1 file changed, 3 insertions(+)
Show changes to compat/win32/pthread.c +3 −0
diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index cf53bc61d82..85f8f7920ce 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -42,10 +42,13 @@ int win32_pthread_join(pthread_t *thread, void **value_ptr)
 	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:
+		/* the wait failed, so do not detach */
 		return err_win_to_posix(GetLastError());
 	}
 }
-- 
gitgitgadget
Junio C Hamano· Jan 6, 2023, 12:28 UTC · re: Rose via GitGitGadget · lore

Re: [PATCH v14 0/2] win32: close handles of threads that have been joined

"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 22 quoted lines
> 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
>
> Seija Kijin (2):
>   win32: prepare pthread.c for change by formatting
>   win32: close handles of threads that have been joined
>
>  compat/win32/pthread.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
>
> base-commit: 4dbebc36b0893f5094668ddea077d0e235560b16
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1406%2FAtariDreams%2Fjoin-v14
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1406/AtariDreams/join-v14
> Pull-Request: https://github.com/git/git/pull/1406
>
> Range-diff vs v13:
>
>  1:  6f89b120641 = 1:  a20eafb9541 win32: prepare pthread.c for change by formatting
>  2:  f126d6416d5 = 2:  aa0319bc08e win32: close handles of threads that have been joined

In general, the cover letter of a reroll should explain why reviewers are asked to look at the updated version instead of the older one, i.e. what changed since the previous iteration and why. For those who followed and/or participated in the review of previous iterations, the range-diff might be sufficient to guess why these changes were made in order to improve what, but for those who are coming late to the party, it would be helpful if you explain what you changed, how and why.

This is true especially if (1) you are rerolling even though the previous round got little reviews or (2) you are resending without any change.

Thanks.

← back to recent threads