Re: [PATCH v2] win32: remove handling for impossible cases in win32_pthread_join
"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.
> 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(-)
Show 19 quoted lines
> 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 ...
Show 5 quoted lines
> + if (value_ptr)
> + *value_ptr = thread->arg;
> +
> + CloseHandle(thread->handle);
> + return 0;
Show 5 quoted lines
> }
>
> pthread_t pthread_self(void)
>
> base-commit: 9a2fb147f2c61d0cab52c883e7e26f5b7948e3ed
