Re: dinput: Create single thread for mouse and keyboard hook procs. [try 2]

[Robert Shearman]

Vitaliy Margolen wrote:
> +static DWORD WINAPI hook_thread_proc(void *param)
> +{
> - static const WCHAR classW[]={'H','o','o','k','_','L','L','_','C','L',0};
> - MSG msg;
> - WNDCLASSEXW wcex;
> - HWND hwnd;
> -
> - memset(&wcex, 0, sizeof(wcex));
> - wcex.cbSize = sizeof(wcex);
> - wcex.lpfnWndProc = (WNDPROC)dinput_hook_WndProc;

Don't case the function pointer here.
> - wcex.lpszClassName = classW;
> - wcex.hInstance = GetModuleHandleW(0);
> -
> - if (!RegisterClassExW(&wcex)) ERR("Error registering window class\n");
> - hwnd = CreateWindowExW(0, classW, NULL, 0, 0, 0, 0, 0, HWND_MESSAGE, NULL, NULL, 0);
> - *(HWND*)param = hwnd;
> -
> - SetEvent(signal_event);
> - if (hwnd)
> - {
> -
>    while (GetMessageW(&msg, 0, 0, 0))
> -
>    {
> -
>        TranslateMessage(&msg);
> -
>        DispatchMessageW(&msg);
> -
>    }
> - }
> - else ERR("Error creating message window\n");
> -
> - DestroyWindow(hwnd);
> - UnregisterClassW(wcex.lpszClassName, wcex.hInstance);
> - return 0;
>
> +}
> -
> +static HWND get_thread_hwnd(void)
> +{
> - static HANDLE hook_thread;
> - static HWND   hook_thread_hwnd;
> -
> - if (!hook_thread)
> - {
> -
>    HANDLE thread;
> -
>    DWORD tid;
> -
>    HWND hwnd;
> -
> -
>    thread = CreateThread(NULL, 0, hook_thread_proc, &hwnd, CREATE_SUSPENDED, &tid);
> -
>    if (InterlockedCompareExchangePointer((PVOID)&hook_thread, thread, 0) != 0)
> -
>    {
> -
>        /* someone beat us here... */
> -
>        TerminateThread(thread, 0);
> -
>        WaitForSingleObject(thread, INFINITE);
> -
>        CloseHandle(thread);
> -
>    }
> -
>    else
> -
>    {
> -
>        signal_event = CreateEventW(NULL, FALSE, FALSE, NULL);
> -
>        ResumeThread(thread);
> -
>        WaitForSingleObject(signal_event, INFINITE);
> -
>        CloseHandle(signal_event);
> -
> -
>        if (!(hook_thread_hwnd = hwnd))
> -
>        {
> -
>            /* Thread failed to create window - reset things so we could
> -
>               try again later */
> -
>            CloseHandle(thread);
> -
>            hook_thread = 0;
> -
>        }
> -
>    }
> - }
> - return hook_thread_hwnd;
>
> +}

This looks racy. You try to clean up in the case where the thread was 
already created, yet some resource like the thread stack won't be 
cleanup up. IMHO, you are better off putting a critical section around 
the accesses to hook_thread and hook_thread_hwnd so you can simplify the 
code a bit.