| Message ID | pull.1406.git.git.1671474876207.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | win32: close handles of threads that have been joined | expand |
On Mon, Dec 19 2022, Rose via GitGitGadget wrote: > 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. */ > + 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?
Am 19.12.22 um 19:34 schrieb Rose via GitGitGadget: > 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 | 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). > + default: > + case WAIT_FAILED: > + /* the function failed so we do not detach */ > + return err_win_to_posix(GetLastError()); > } Good. -- Hannes
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()); } }