From: Junio C Hamano Date: Tue, 18 Nov 2025 17:02:36 GMT Subject: Re: [PATCH v2] win32: remove handling for impossible cases in win32_pthread_join Message-ID: In-Reply-To: "AZero13 via GitGitGadget" writes: > From: Greg Funni 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 > 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.