[PATCH 0/1] MR8066: mmdevapi: Avoid race between main and notify thread.

[Bernhard Übelacker]

The gitlab runners where showing a few flaky crashes in e.g. tests:
  dmsynth:dmsynth dmusic:dmusic winmm:midi

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58233

This patch attempts to wait at the DRV_CLOSE call for the thread to finish.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066

[PATCH 1/1] mmdevapi: Avoid race between main and notify thread.

[Bernhard Übelacker]

From: Bernhard Übelacker <bernhardu(a)mailbox.org>

The gitlab runners where showing a few flaky crashes in e.g. tests:
  dmsynth:dmsynth dmusic:dmusic winmm:midi

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=58233
---
 dlls/mmdevapi/main.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/dlls/mmdevapi/main.c b/dlls/mmdevapi/main.c
index 551d53ccb7f..c61afe06396 100644
--- a/dlls/mmdevapi/main.c
+++ b/dlls/mmdevapi/main.c
@@ -307,6 +307,7 @@ static IClassFactoryImpl MMDEVAPI_CF[] = {
 };
 
 static INIT_ONCE init_once = INIT_ONCE_STATIC_INIT;
+static HANDLE notify_thread_handle;
 
 HRESULT WINAPI DllGetClassObject(REFCLSID rclsid, REFIID riid, LPVOID *ppv)
 {
@@ -384,14 +385,17 @@ LRESULT WINAPI DriverProc( DWORD_PTR id, HANDLE driver, UINT msg, LPARAM param1,
 
         params.err = &err;
         MIDI_CALL( midi_init, ¶ms );
-        if (err == DRV_SUCCESS) CloseHandle( CreateThread( NULL, 0, notify_thread, NULL, 0, NULL ));
+        if (err == DRV_SUCCESS) notify_thread_handle = CreateThread( NULL, 0, notify_thread, NULL, 0, NULL );
         return err;
     }
     case DRV_FREE:
+    case DRV_CLOSE:
         MIDI_CALL( midi_release, NULL );
+        WaitForSingleObject(notify_thread_handle, INFINITE);
+        CloseHandle(notify_thread_handle);
+        notify_thread_handle = NULL;
         return 1;
     case DRV_OPEN:
-    case DRV_CLOSE:
     case DRV_QUERYCONFIGURE:
     case DRV_CONFIGURE:
         return 1;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/8066

[Huw Davies (＠huw)]

Huw Davies (@huw) commented about dlls/mmdevapi/main.c:
> params.err = &err;
>          MIDI_CALL( midi_init, ¶ms );
> -        if (err == DRV_SUCCESS) CloseHandle( CreateThread( NULL, 0, notify_thread, NULL, 0, NULL ));
> +        if (err == DRV_SUCCESS) notify_thread_handle = CreateThread( NULL, 0, notify_thread, NULL, 0, NULL );
>          return err;
>      }
>      case DRV_FREE:
> +    case DRV_CLOSE:
>          MIDI_CALL( midi_release, NULL );
> +        WaitForSingleObject(notify_thread_handle, INFINITE);
> +        CloseHandle(notify_thread_handle);
> +        notify_thread_handle = NULL;

Should we be doing this for both `DRV_FREE` and `DRV_CLOSE`?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066#note_104078

[Bernhard Übelacker]

On Wed May 21 09:30:48 2025 +0000, Huw Davies wrote:
> Should we be doing this for both `DRV_FREE` and `DRV_CLOSE`?
> Sorry for the delay.

Before only the `DRV_FREE` case contained the call to `MIDI_CALL( midi_release, NULL );`, whis is causes the thread leave its loop. But in this failure case through `CloseDriver` just the `DRV_CLOSE` case is reached, so I put both together. If we just wait in the `DRV_CLOSE` case the thread never leaves its loop.

I can make the close case distinct with a second line midi_release and the wait, and not touching `DRV_FREE` if this is desired?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066#note_104235

[eric pouech (＠epo)]

On Thu May 22 18:50:27 2025 +0000, Bernhard Übelacker wrote:
> Sorry for the delay.
> Before only the `DRV_FREE` case contained the call to `MIDI_CALL(
> midi_release, NULL );`, whis is causes the thread leave its loop. But in
> this failure case through `CloseDriver` just the `DRV_CLOSE` case is
> reached, so I put both together. If we just wait in the `DRV_CLOSE` case
> the thread never leaves its loop.
> I can make the close case distinct with a second line midi_release and
> the wait, and not touching `DRV_FREE` if this is desired?
> actually (from winmm.dll standpoint):

- `DRV_LOAD/DRV_FREE` are paired (first is sent when DLL containing driver is loaded, and second when it's about to be unloaded)
- `DRV_OPEN/DRV_CLOSE` are paired: a driver can support different instances (eg. with different parameters), and they are used to create/destroy such an instance

so it makes no sense to do something on `DRV_LOAD` and its counterpart on `DRV_CLOSE` (even if for mmdevapi it's likely we only have one instance created) 

did you investigate why the `DRV_FREE` message wasn't sent from `winmm.CloseDriver()`?

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066#note_104313