Fix issues with DirectSound thread cleanup code #922
Conversation
Thanks for offering the fix. Could you create an Issue, or two, so that we can discuss the bug? |
|
I agree that the portaudio code is not consistent with the microsoft API docs at https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/endthread-endthreadex?view=msvc-170 I can see where the confusion comes from:
It also looks like the old code was not consistently using the macros supplied for creating and closing the thread (the macros exist for the purpose of cygwin compatibility when beginthreadex/endthreadex is not used). Ideally I think the macros should be retained and fixed rather than eliminating them, since they clearly abstract the differences between the two runtimes. |
The previous code refrained from calling CloseHandle on _beingthreadex()-created threads, citing "documentation". Actually the official docs suggest the exact opposite: https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/endthread-endthreadex > when you use _beginthreadex and _endthreadex, you must close the > thread handle by calling the Win32 CloseHandle API This means the previous code would leak the thread handle. This can be confirmed empirically by looking at the process Win32 handle list in e.g. Process Explorer. Fixes PortAudio#925
The previous code would return from Pa_StopStream() when the streaming thread signals that it is done. This is racy, because the streaming thread is technically still running code by that point. If, for example, the PortAudio user decides to unload the entire PortAudio DLL as soon as Pa_StopStream() returns, hilarity could ensue. Instead we can simply wait on the thread handle itself, which is both safer and simpler. Fixes PortAudio#926
Done by preserving the (I would argue that these macros don't really make sense in practice and we should get rid of them, because they are abstracting away differences that don't really exist - the official docs of |
There was a problem hiding this comment.
IMPORTANT: DO NOT SQUASH when merging.
I have finally made time to review this properly. Overall it looks fine, although it would have been easier to deal with if it was two separate PRs.
7398279 (removal of event signaling thread completion) is 100% ok by me.
4a2fef4 (match _beginthreadex with CloseThread) appears correct to me. Not sure the fix is ideal, but ok.
| @@ -3051,9 +3051,7 @@ static PaError StartStream( PaStream *s ) | |||
| #ifndef PA_WIN_DS_USE_WMME_TIMER | |||
| if( stream->processingThread ) | |||
| { | |||
| #ifdef CLOSE_THREAD_HANDLE | |||
There was a problem hiding this comment.
The purpose of #ifdef CLOSE_THREAD_HANDLE is to allow the macro to be a no-op. Given that there is now no situation where it would be a no-op I am not sure that the macro should be present at all -- but maybe it makes sense to keep it to provide some hint that it is dependent on the way the thread is created.
|
Merged. Thanks @dechamps |
This PR fixes a couple of issues with the way the DirectSound Host API cleans up its streaming thread, and also simplifies the code as a nice side effect.
Details can be found in the commit messages.