[PATCH v2 0/1] MR1455: dinput: Wait on event instead of thread on stop

[Fabian Maurer (＠DarkShadow44)]

Since we stop the thread in DllMain, we always haev the loaderlock.
Thread shutdown also requires the loader lock, so we will always time out
So just wait until all cleanup is done, the thread itself can exit later

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52314
Signed-off-by: Fabian Maurer

--
  v2: dinput: Wait on event instead of thread on stop

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

[PATCH v2 1/1] dinput: Wait on event instead of thread on stop

[Fabian Maurer]

From: Fabian Maurer

Since we stop the thread in DllMain, we always haev the loaderlock.
Thread shutdown also requires the loader lock, so we will always time out
So just wait until all cleanup is done, the thread itself can exit later

Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=52314
Signed-off-by: Fabian Maurer 
---
 dlls/dinput/dinput_main.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/dlls/dinput/dinput_main.c b/dlls/dinput/dinput_main.c
index d3db22c70b5..dc0de9c44f9 100644
--- a/dlls/dinput/dinput_main.c
+++ b/dlls/dinput/dinput_main.c
@@ -1227,7 +1227,7 @@ static DWORD WINAPI dinput_thread_proc( void *params )
     static HHOOK kbd_hook, mouse_hook;
     struct dinput_device *impl, *next;
     SIZE_T events_count = 0;
-    HANDLE finished_event;
+    HANDLE finished_event = NULL;
     DWORD ret;
     MSG msg;

@@ -1320,6 +1320,7 @@ static DWORD WINAPI dinput_thread_proc( void *params )
 done:
     DestroyWindow( di_em_win );
     di_em_win = NULL;
+    SetEvent(finished_event);
     return 0;
 }

@@ -1347,9 +1348,13 @@ static void dinput_thread_start(void)

static void dinput_thread_stop(void)
 {
-    PostThreadMessageW( dinput_thread_id, WM_USER + 0x10, 0, 0 );
-    if (WaitForSingleObject( dinput_thread, 500 ) == WAIT_TIMEOUT)
-        WARN("Timeout while waiting for internal thread\n");
+    HANDLE finished_event = CreateEventW( NULL, FALSE, FALSE, NULL );
+
+    PostThreadMessageW( dinput_thread_id, WM_USER + 0x10, 0, (LPARAM)finished_event );
+
+    WaitForSingleObject( finished_event, INFINITE );
+
+    CloseHandle( finished_event );
     CloseHandle( dinput_thread );
 }

[PATCH v2 1/1] dinput: Wait on event instead of thread on stop

[Marvin]

Hi,

It looks like your patch introduced the new failures shown below.
Please investigate and fix them before resubmitting your patch.
If they are not new, fixing them anyway would help a lot. Otherwise
please ask for the known failures list to be updated.

The tests also ran into some preexisting test failures. If you know how
to fix them that would be helpful. See the TestBot job for the details:

The full results can be found at:
https://testbot.winehq.org/JobDetails.pl?Key=126509

Your paranoid android.

=== debian11 (32 bit report) ===

d3d8:
stateblock: Timeout
visual: Timeout

d3d9:
d3d9ex: Timeout
device: Timeout
stateblock: Timeout
visual: Timeout

d3dcompiler_43:
asm: Timeout
blob: Timeout
hlsl_d3d11: Timeout
hlsl_d3d9: Timeout
reflection: Timeout

d3dcompiler_46:
asm: Timeout
blob: Timeout
hlsl_d3d11: Timeout
hlsl_d3d9: Timeout
reflection: Timeout

d3dcompiler_47:
asm: Timeout
blob: Timeout
hlsl_d3d11: Timeout
hlsl_d3d9: Timeout
reflection: Timeout

d3drm:
d3drm: Timeout
vector: Timeout

Report validation errors:
d3dx10: Timeout

=== debian11 (build log) ===

WineRunWineTest.pl:error: The task timed out

[Rémi Bernon (＠rbernon)]

This won't work as there's then a small chance that the thread sets the event, wakes the waiting thread which then proceed to unloading the module, before the thread returns from SetEvent, causing a segfault when it does.

[Fabian Maurer (＠DarkShadow44)]

@rbernon
Ah, I overlooked that...
Would a function similar to FreeLibraryAndExitThread be acceptable? Like __wine_set_signal_and_exit_thread? Then we set a signal and keep waiting for the loader_lock inside Wine code, so no race conditions there.

[Rémi Bernon (＠rbernon)]

On Mon Nov 21 07:04:16 2022 +0000, Fabian Maurer wrote:
> @rbernon
> Ah, I overlooked that...
> Would a function similar to FreeLibraryAndExitThread be acceptable? Like
> __wine_set_signal_and_exit_thread? Then we set a signal and keep waiting
> for the loader_lock inside Wine code, so no race conditions there.

I don't think it needs a wine specific function and I don't think it's the right way to fix it. The right fix would probably be to keep the thread alive only when there's dinput instances alive but that's much more changes.

[Fabian Maurer (＠DarkShadow44)]

On Mon Nov 21 07:04:16 2022 +0000, Rémi Bernon wrote:
> I don't think it needs a wine specific function and I don't think it's
> the right way to fix it. The right fix would probably be to keep the
> thread alive only when there's dinput instances alive but that's much
> more changes.

Alright, makes sense. I guess I better leave this to you then.

[PATCH v2 0/1] MR1455: dinput: Wait on event instead of thread on stop - closed

[Fabian Maurer (＠DarkShadow44)]

This merge request was closed by Fabian Maurer.