[PATCH] win32: remove handling for impossible cases in win32_pthread_join

[PATCH] win32: remove handling for impossible cases in win32_pthread_join

[AZero13 via GitGitGadget]

From: AZero13 <gfunni234@gmail.com>

WAIT_FAILED is the only real possible error here.

Signed-off-by: Greg Funni <gfunni234@gmail.com>
---
    win32: remove handling for impossible cases in win32_pthread_join
    
    WAIT_FAILED is the only real possible error here.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v1
Pull-Request: https://github.com/git/git/pull/2102

 compat/win32/pthread.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 58980a529c..54c43b4146 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr 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 */
+	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == 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: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
gitgitgadget

Re: [PATCH] win32: remove handling for impossible cases in win32_pthread_join

[Junio C Hamano]

"AZero13 via GitGitGadget" <gitgitgadget@gmail.com> writes:

First, welcome to the Git development community (I do not think I
have seen e-mails from this address).

Common to all three patches, this name on the in-body From: line ...

> From: AZero13 <gfunni234@gmail.com>

... that overrides what is on the e-mail header (which always points at
GitGitGadget address if you are using GitGitGadget) comes from the
user.name configuration that was used when you recorded the commit.
It has to match the name you use to sign-off the patch, when you are
sending your own patch, which is ...

> WAIT_FAILED is the only real possible error here.
>
> Signed-off-by: Greg Funni <gfunni234@gmail.com>

... on this line.  Perhaps do this once

    $ git config user.name "Greg Funni"

in the repository you are doing the public work on Git, and then
check out the commit you committed under the AZero13 identity and

    $ git commit --amend --reset-author

After that, force update the pull request would be necessary to
correct them.

Other two patches I saw no issues with, but this one needs a bit
more than the above "is the only real possible" single liner
explanation.  For example, somebody who is clueless on Win32 API
(like me) would visit

  https://learn.microsoft.com/en-us/windows/win32/api/synchapi/nf-synchapi-waitforsingleobjectex

and wonder why among those 5 listed, WAIT_FAILED is the only one
that can be returned.  Something like

    WAIT_TIMEOUT would not be returned as the INFINITE is given to
    the call.

would have been a reasonable explanation if this patch were removing
the case that reacts to that result (but the original does not react
to it, probably they already knew that it is impossible).

So the original code used to futz with *value_ptr when the result is
WAIT_OBJECT_0, but left *value_ptr intact when the result is
WAIT_ABANDONED.  And with these results, it called CloseHandle().

Other results, including but not limited to WAIT_FAILED, returned
error code without calling CloseHandle().  The updated code does the
"return error without calling CloseHandle()" only for WAIT_FAILED,
and for all other results, including WAIT_OBJECT_0 and
WAIT_ABANDONED, does the same futzing with *value_ptr.  Are all
these changes intended, and why are they good things?

Your proposed log message should answer these questions for future
readers of "git log", because for them, unlike me who can ask these
questions real-time to you in a patch-submitter and reviewer
relationship, it is not practical to ask you.

>  compat/win32/pthread.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)
>
> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 58980a529c..54c43b4146 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr 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 */
> +	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == 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: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed

[PATCH v2] win32: remove handling for impossible cases in win32_pthread_join

[AZero13 via GitGitGadget]

From: Greg Funni <gfunni234@gmail.com>

WAIT_FAILED is the only real possible error here.

WAIT_TIMEOUT would not be returned as the INFINITE
is given to the call.

WAIT_ABANDONED would be returned if the handle
pointed to a mutex object that was not released
by the thread that owned the mutex object before
the owning thread terminated.

WAIT_IO_COMPLETION would not be returned because
we pass FALSE so the wait is not alertable.

Signed-off-by: Greg Funni <gfunni234@gmail.com>
---
    win32: remove handling for impossible cases in win32_pthread_join
    
    WAIT_FAILED is the only real possible error here.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v2
Pull-Request: https://github.com/git/git/pull/2102

Range-diff vs v1:

 1:  73fae07514 ! 1:  e0d6b15093 win32: remove handling for impossible cases in win32_pthread_join
     @@
       ## Metadata ##
     -Author: AZero13 <gfunni234@gmail.com>
     +Author: Greg Funni <gfunni234@gmail.com>
      
       ## Commit message ##
          win32: remove handling for impossible cases in win32_pthread_join
      
          WAIT_FAILED is the only real possible error here.
      
     +    WAIT_TIMEOUT would not be returned as the INFINITE
     +    is given to the call.
     +
     +    WAIT_ABANDONED would be returned if the handle
     +    pointed to a mutex object that was not released
     +    by the thread that owned the mutex object before
     +    the owning thread terminated.
     +
     +    WAIT_IO_COMPLETION would not be returned because
     +    we pass FALSE so the wait is not alertable.
     +
          Signed-off-by: Greg Funni <gfunni234@gmail.com>
      
       ## compat/win32/pthread.c ##


 compat/win32/pthread.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 58980a529c..54c43b4146 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr 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 */
+	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == 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: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
gitgitgadget

[PATCH v3] win32: remove handling for impossible cases in win32_pthread_join

[AZero13 via GitGitGadget]

From: Greg Funni <gfunni234@gmail.com>

WAIT_FAILED is the only real possible error here.

WAIT_TIMEOUT would not be returned as the INFINITE
is given to the call.

WAIT_ABANDONED would be returned if the handle
pointed to a mutex object that was not released
by the thread that owned the mutex object before
the owning thread terminated.

WAIT_IO_COMPLETION would not be returned because
we pass FALSE so the wait is not alertable.

Signed-off-by: Greg Funni <gfunni234@gmail.com>
---
    win32: remove handling for impossible cases in win32_pthread_join
    
    WAIT_FAILED is the only real possible error here.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-2102%2FAZero13%2Fpatch-1-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-2102/AZero13/patch-1-v3
Pull-Request: https://github.com/git/git/pull/2102

Range-diff vs v2:

 1:  e0d6b15093 = 1:  20f943570f win32: remove handling for impossible cases in win32_pthread_join


 compat/win32/pthread.c | 20 +++++++-------------
 1 file changed, 7 insertions(+), 13 deletions(-)

diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
index 58980a529c..54c43b4146 100644
--- a/compat/win32/pthread.c
+++ b/compat/win32/pthread.c
@@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr 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 */
+	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == 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: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
-- 
gitgitgadget

Re: [PATCH v2] win32: remove handling for impossible cases in win32_pthread_join

[Junio C Hamano]

"AZero13 via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Greg Funni <gfunni234@gmail.com>

Looking much better, but there still are some puzzlement left.

> WAIT_FAILED is the only real possible error here.
>
> WAIT_TIMEOUT would not be returned as the INFINITE
> is given to the call.

OK.

> WAIT_ABANDONED would be returned if the handle
> pointed to a mutex object that was not released
> by the thread that owned the mutex object before
> the owning thread terminated.

... and we know the handle we are passing to WaitForSingleObject()
is a Thread, not a Mutex, so this error condition is irrelevant?

OK.

> WAIT_IO_COMPLETION would not be returned because
> we pass FALSE so the wait is not alertable.

FALSE where?  Ah, this can be done with WaitForSingleObjectEx, but
WaitForSingleObject() is what we call here, so WAIT_IO_COMPLETION
won't be returned with or without FALSE.  Is there a reason why you
want to change the code to call Ex variant (the manual page tells us
to use it _if_ we want to enter an alertable wait state, and I am
assuming that we are not interested in doing so)?

In any case, among the four possible return values from
WaitForSingleObject(), we know WAIT_ABANDONED and WAIT_TIMEOUT will
not be relevant for this code path.

Because WAIT_OBJECT_0 is the cryptic synonym for "Success!" for this
call, WAIT_FAILED is indeed the only possible error here, just like
you said at the beginning.

I think it is easier to understand for mere-mortal readers like me,
who are not familiar with Win32 API, if we explained this change
more like:

  Subject: [PATCH] win32: simplify win32_pthread_join() error handling

  Among the four possible result WaitForSingleObject() can return,
  WAIT_TIMEOUT and WAIT_ABANDONED are not relevant in this code
  path, because we do not ask for the call to time-out, and we do
  not pass a mutex object to the call (we are passing a thread
  object).

  Simplify the code to

  - return an error without closing the handle if the call failed
    (i.e., returns WAIT_FAILED that is not zero);

  - otherwise, WAIT_OBJECT_0 (which is 0) is returned to signal a
    success.  Do exactly what the original code did in this case.

What do you think?

> Signed-off-by: Greg Funni <gfunni234@gmail.com>
>  compat/win32/pthread.c | 20 +++++++-------------
>  1 file changed, 7 insertions(+), 13 deletions(-)


> diff --git a/compat/win32/pthread.c b/compat/win32/pthread.c
> index 58980a529c..54c43b4146 100644
> --- a/compat/win32/pthread.c
> +++ b/compat/win32/pthread.c
> @@ -37,20 +37,14 @@ int pthread_create(pthread_t *thread, const void *attr 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:

And the above is what the patch simplifies away, which is great.

> -		/* the wait failed, so do not detach */

I think this comment is worth keeping (I am assuming "do not detach"
refers to the fact that CloseHandle(thread->handle) is not called in
the error case).

> +	if (WaitForSingleObjectEx(thread->handle, INFINITE, FALSE) == WAIT_FAILED)
>  		return err_win_to_posix(GetLastError());

The change is based on out belief that WAIT_FAILED is the only
possible error from this call.  Even if our belief turns out to be
wrong, we would want to take the error code path, wouldn't we?  IOW,
I think the above should be more like

	if (WaitForSingleObject(thread->handle, INFINITE))
		/* the wait failed; do not detach */
		return err_win_to_posix(GetLastError());

i.e., if we get an error, report the error to the caller, regardless
of what kind of an error it is, even though we expect it to be a
WAIT_FAILED.  And the case this if() condition does not catch is a
successful wait (i.e., WAIT_OBJECT_0), which is handled ...

> +	if (value_ptr)
> +		*value_ptr = thread->arg;
> +
> +	CloseHandle(thread->handle);
> +	return 0;

... exactly as before.

>  }
>  
>  pthread_t pthread_self(void)
>
> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed

Looking good.  Thanks.