[PATCH v2 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.

--
  v2: mmdevapi: Avoid race between main and notify thread.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066

[PATCH v2 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 | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/dlls/mmdevapi/main.c b/dlls/mmdevapi/main.c
index 551d53ccb7f..ad618626b8a 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,11 +385,14 @@ 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:
         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:
-- 
GitLab

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

[Bernhard Übelacker]

On Fri May 23 07:32:07 2025 +0000, eric pouech wrote:
> 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()`?
> I was wrong and the `DRV_FREE` really is sent. In this case I guess moving the wait from the `DRV_CLOSE` to `DRV_FREE` is better? I also could not sense an infinite wait with it today.

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

[Bernhard Übelacker]

v2:
- move the wait to `DRV_FREE` alone.

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

[Rémi Bernon]

Rémi Bernon (@rbernon) 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:
>          MIDI_CALL( midi_release, NULL );
> +        WaitForSingleObject(notify_thread_handle, INFINITE);
> +        CloseHandle(notify_thread_handle);
> +        notify_thread_handle = NULL;

```suggestion:-2+0
        WaitForSingleObject( notify_thread_handle, INFINITE );
        CloseHandle( notify_thread_handle );
        notify_thread_handle = NULL;
```

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

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

[Rémi Bernon]

This merge request was approved by Rémi Bernon.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066

[Rémi Bernon]

Otherwise LGTM. Note that it's possible for the wait to be blocked forever, if the notify thread is calling a blocking `DriverCallback` while being notified to quit, only if a client function was provided through `midiOutOpen` / `midiInOpen`, but at least we never seem to do that in our code and there's so reason to assume these callbacks would block.

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

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

[Huw Davies (＠huw)]

This merge request was approved by Huw Davies.
-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/8066