Use standard C++ for DeviceThreadBase and MMThreadLock implementation - #973
Merged
marktsuchida merged 6 commits intoJul 20, 2026
Merged
Conversation
On Windows, the handle returned from CreateThread() must be closed; we were leaking it. On POSIX, we need to either join or detach the thread, or else the kernel keeps a record of the thread until process exit. In addition to plugging those leaks (by detaching in the destructor), - Also avoid a similar leak when re-activating - Fail early and definitely on double-join (a programming error) - Change activate() to return void and make non-virtual (no code in our codebase uses the return value or overrides activate()) - Use modern way to disable copy/move construction/assignment (= delete)
Much simpler. Behavioral changes: errors now throw rather than silently continue or terminate. In practice this is probably a good thing: typical callers won't swallow exceptions and we'll crash with "uncaught C++ exception", which is better than continuing or terminating with no information. In any case errors are either programming errors or rare resource exhaustions.
No behavioral change.
Not used, and cannot think of a valid use case. Note that std::lock_guard has no analogous function.
Previously ignored (rare) errors will now throw.
No longer needed in DeviceThreads.h, now that we use standard C++ thread/mutex. Lots of device adapters relied on transitive inclusion (including via DeviceBase.h, which includes DeviceThreads.h), so required fixing. In some cases, defining `WIN32_LEAN_AND_MEAN` was the critical part (to prevent Windows.h from defining `byte`, which clashes with C++17 `std::byte` if `using namespace std` is used). This includes the MCCDAQ vendor header which includes Windows.h. For IntegratedLaserEngine, a member function GetCurrentTime had to be renamed, because Windows.h defines a macro of that name. The reason it was working previously was that all occurrences of GetCurrentTime were being replaced by the same name (GetTickCount), but that was extremely fragile. Some additional cleanup of #includes and adjacent directives is included.
marktsuchida
force-pushed
the
fix-device-thread-leak
branch
from
July 20, 2026 20:42
d9bb064 to
996540c
Compare
Member
Author
|
There were no missing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #971.
Retire the Win32 and pthreads types/calls and replace with
std::threadandstd::recursive_mutex.No longer leak a thread handle (Windows) or joinable thread (POSIX).
Patch device adapters that broke because MMDevice no longer includes Windows.h or defines
WIN32_LEAN_AND_MEAN.No device ABI changes. Source-incompatible changes (no in-tree adapters affected):
DeviceThreadBase::activate()is no longervirtualand now returnsvoidrather thanint(no adapter overrides this function or reads its return value)MMThreadGuard::isLocked()has been deleted (no adapter calls it)TODO: