Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix leaking thread handles on Windows #3147

Merged
merged 1 commit into from Jun 2, 2022
Merged

fix leaking thread handles on Windows #3147

merged 1 commit into from Jun 2, 2022

Conversation

ghost
Copy link

@ghost ghost commented May 30, 2022

Fix #3051. On Windows, thread handles should be closed explicitly.

  1. ZSTD_pthread_join() function is called when shuting down the threads queue, so no need to set thread.handle to 0.
  2. The handles are created by _beginthreadex() function, there is an example at the bottom of this MSDN page that matches zstd code.

A Python reproduce code (with pyzstd module), it uses 12 threads to compress INPUT file:

import os
from ctypes import windll, wintypes, byref

import pyzstd

INPUT = r'e:\a.tar'
OUTPUT = INPUT + '.zst'

# Get the handle number of this process
def get_handle_count():
    GetCurrentProcess = windll.kernel32.GetCurrentProcess
    GetCurrentProcess.argtypes = ()
    GetCurrentProcess.restype = wintypes.HANDLE

    GetProcessHandleCount = windll.kernel32.GetProcessHandleCount
    GetProcessHandleCount.argtypes = (wintypes.HANDLE, wintypes.PDWORD)
    GetProcessHandleCount.restype = wintypes.BOOL

    count = wintypes.DWORD(0)
    handle = GetCurrentProcess()
    ret = GetProcessHandleCount(handle, byref(count))
    if not ret:
        raise Exception('GetProcessHandleCount() failed')

    return count.value

for i in range(50):
    with open(INPUT, 'rb') as ifh:
        with open(OUTPUT, 'wb') as ofh:
            pyzstd.compress_stream(ifh, ofh,
                                   level_or_option={pyzstd.CParameter.nbWorkers:12}
                                   )
    os.remove(OUTPUT)
    print('round:', i, 'handles:', get_handle_count())

Reproduce code output. Before this PR, it leaks 12 handles per round:

                before         after
round: 0     handles: 137   handles: 125
round: 1     handles: 149   handles: 125
round: 2     handles: 161   handles: 125
round: 3     handles: 173   handles: 125
round: 4     handles: 185   handles: 125
round: 5     handles: 197   handles: 125
round: 6     handles: 209   handles: 125
round: 7     handles: 221   handles: 125
round: 8     handles: 233   handles: 125
round: 9     handles: 245   handles: 125
round: 10    handles: 257   handles: 125
round: 11    handles: 269   handles: 125
round: 12    handles: 281   handles: 125
round: 13    handles: 293   handles: 125
round: 14    handles: 305   handles: 125
round: 15    handles: 317   handles: 125
round: 16    handles: 329   handles: 125
round: 17    handles: 341   handles: 125
round: 18    handles: 353   handles: 125
round: 19    handles: 365   handles: 125
round: 20    handles: 377   handles: 125
round: 21    handles: 389   handles: 125
round: 22    handles: 401   handles: 125
round: 23    handles: 413   handles: 125
round: 24    handles: 425   handles: 125
round: 25    handles: 437   handles: 125
round: 26    handles: 449   handles: 125
round: 27    handles: 461   handles: 125
round: 28    handles: 473   handles: 125
round: 29    handles: 485   handles: 125
round: 30    handles: 497   handles: 125
round: 31    handles: 509   handles: 125
round: 32    handles: 521   handles: 125
round: 33    handles: 533   handles: 125
round: 34    handles: 545   handles: 125
round: 35    handles: 557   handles: 125
round: 36    handles: 569   handles: 125
round: 37    handles: 581   handles: 125
round: 38    handles: 593   handles: 125
round: 39    handles: 605   handles: 125
round: 40    handles: 617   handles: 125
round: 41    handles: 629   handles: 125
round: 42    handles: 641   handles: 125
round: 43    handles: 653   handles: 125
round: 44    handles: 665   handles: 125
round: 45    handles: 677   handles: 125
round: 46    handles: 689   handles: 125
round: 47    handles: 701   handles: 125
round: 48    handles: 713   handles: 125
round: 49    handles: 725   handles: 125

On Windows, thread handle should be closed explicitly.

Co-authored-by: luben karavelov <luben@users.noreply.github.com>
@Cyan4973
Copy link
Contributor

Great find @animalize !
Looks good to me

cc @terrelln

@ghost
Copy link
Author

ghost commented Jun 1, 2022

On Windows, threads are created by _beginthreadex() function (not _beginthread()):

thread->handle = (HANDLE) _beginthreadex(NULL, 0, worker, thread, 0, NULL);


There is another review resource in the MSDN page of _endthread(), _endthreadex() functions:

You can call _endthread or _endthreadex explicitly to terminate a thread; however, _endthread or _endthreadex is called automatically when the thread returns from the routine passed as a parameter to _beginthread or _beginthreadex. Terminating a thread with a call to _endthread or _endthreadex helps ensure proper recovery of resources allocated for the thread.

So no need to call _endthreadex(), as zstd code.

_endthread automatically closes the thread handle. (This behavior differs from the Win32 ExitThread API.) Therefore, when you use _beginthread and _endthread, don't explicitly close the thread handle by calling the Win32 CloseHandle API.

Like the Win32 ExitThread API, _endthreadex doesn't close the thread handle. Therefore, when you use _beginthreadex and _endthreadex, you must close the thread handle by calling the Win32 CloseHandle API.

Zstd is using _beginthreadex(), so need to call CloseHandle(), as this PR.

@terrelln
Copy link
Contributor

terrelln commented Jun 2, 2022

Thanks for the fix @animalize!

@terrelln terrelln merged commit 9f346db into facebook:dev Jun 2, 2022
@Cyan4973 Cyan4973 mentioned this pull request Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows: leaking thread handles
4 participants