win32: remove handling for impossible cases in win32_pthread_join

From: AZero13 <[email protected]>

WAIT_FAILED is the only real possible error here.

---
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(-)

index 58980a529c..54c43b4146 100644
@@ -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

"AZero13 via GitGitGadget" <[email protected]> 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 ...

... 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 ...

... 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.

From: Greg Funni <[email protected]>

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.

---
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 <[email protected]>
+Author: Greg Funni <[email protected]>

## 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.
+

## compat/win32/pthread.c ##

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

index 58980a529c..54c43b4146 100644
@@ -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

From: Greg Funni <[email protected]>

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.

---
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(-)

index 58980a529c..54c43b4146 100644
@@ -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

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

OK.

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

OK.

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?

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

Looking good. Thanks.