-1

I have a thread which creates a hidden window for the purpose of receiving WinAPI messages based on power state. I need to get the HWND of the created window from the thread so that I can throw a WM_QUIT message to close the window and end the thread gracefully:

Main:

HWND hiddenWindowHandle = NULL;
HANDLE PowerWindowThreadHandle = (HANDLE)_beginthreadex(0, 0, &windowsPowerThread, (void*)&hiddenWindowHandle, 0, 0);

Thread:

unsigned int __stdcall windowsPowerThread(void* data)
{
    HWND hiddenWindowHandle = createHiddenWindow();
    HWND hwHandle = *(HWND*)data;
    hwHandle = hiddenWindowHandle;
    ...

The problem is that the hiddenWindowHandle is not being updated with the generated HWND.

I have verified in the thread that it's being created, and I've verified that I'm not trying to access the handle before the thread creates it.

What am I missing here?

Connor Spangler
  • 805
  • 2
  • 12
  • 29
  • How exactly do you make sure that the reading thread waits until the value is available and perform the necessary synchronization? If the first thing the other thread does is create that window, why not create the window in the main thread and pass the handle as argument to the thread function!? That'd eliminate all need for synchronization… – Michael Kenzel Sep 16 '18 at 23:31
  • Because I print the validity of the `HWND` in the thread, and in main have a loop I break via a `cin` after which I print the value as it's seen from main. The thread shows a valid handle, I press enter, main doesn't. The solution you mention of course would work, but at the moment I want to know why *this* solution (which I've seen in multiple answers and forums) doesn't work. – Connor Spangler Sep 16 '18 at 23:32
  • The code in question is here: https://github.com/cspang1/cfg-switcher/tree/thread – Connor Spangler Sep 16 '18 at 23:38
  • 1
    Basic coding bug, you are only updating the local variable. Use HWND* phwnd = (HWND*)data; *phwnd = hiddenWindowHandle; And make sure that the the other hiddenWindowHandle variable (pick a different name!) is not a local variable, its address needs to stay valid while the thread is running. – Hans Passant Sep 17 '18 at 00:13
  • Use `std::promise` and `std::future`. – Jonathan Potter Sep 17 '18 at 03:47
  • 1
    You are trying to implement the wrong solution (see [Why is there a special PostQuitMessage function?](https://blogs.msdn.microsoft.com/oldnewthing/20051104-33/?p=33453)). A better alternative would be to use a (named) event, signaled from the UI thread when it's time to shut down that thread, and change your thread's message loop to use [MsgWaitForMultipleObjects](https://docs.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-msgwaitformultipleobjects). Added bonus: You don't have to communicate any information between threads, or expose an implementation detail (the window). – IInspectable Sep 17 '18 at 07:37
  • Besides, you do not destroy a window [by calling `DeleteObject`](https://github.com/cspang1/cfg-switcher/blob/0aaea2a4f36f2e732b29098b7222e28a04c6940a/cfg-switcher/PowerWindowThread.cpp#L30). You destroy a window by calling [DestroyWindow](https://learn.microsoft.com/en-us/windows/desktop/api/winuser/nf-winuser-destroywindow), as explicitly noted in the documentation for `CreateWindowEx`. And this is how you create a [message-only window](https://learn.microsoft.com/en-us/windows/desktop/winmsg/window-features#message-only-windows). – IInspectable Sep 17 '18 at 07:48
  • Thanks for your inputs, but I'm aware of all of that as it's in my original implementation. The only question was of getting a handle to the window from the thread that created it so it could be closed when the main thread decided it was time to. – Connor Spangler Sep 17 '18 at 07:50
  • 1
    My suggestion accomplishes **exactly** what you are trying to accomplish. Without the need to communicate *any* information. And it doesn't invert responsibilities (like the code you are trying to implement). With your proposed solution, the window suddenly becomes responsible for cleaning up the thread that owns it. If you do not want to confuse readers of your code, follow that model: Windows are owned by threads. If you want a thread to terminate, signal an event, and have the thread cleanup the resources it owns. – IInspectable Sep 17 '18 at 09:54
  • @IInspectable ah ok interesting, I'm definitely interested in what you propose. Once I finish my current features I'll circle back to this. When you say "UI" thread, are you referring to the "main" thread? I'm only creating a single thread (the hidden message window) from int main(). – Connor Spangler Sep 17 '18 at 15:55

1 Answers1

1

Your code lacks the necessary synchronization. What you have here is a data race. Thus, what you get is strictly undefined behavior. What will most likely happen is that the compiler simply does not re-fetch the value of hiddenWindowHandle from memory in every iteration of the loop, since it can simply assume that the value does not change. One possible solution would be to make hiddenWindowHandle an std::atomic and have the main thread perform a busy wait until the value changes from NULL. Alternatively, you could put all access to the shared variable into a critical section locked by a mutex, or use a condition variable to wait for the value to be available.

Edit based on comments:

So if I understand your code correctly, the thread that creates the window receives a pointer to the result variable in the form of a void* and then tries to communicate the result like so:

unsigned int __stdcall windowsPowerThread(void* data)
{
    …
    HWND hwHandle = *(HWND*)data;
    hwHandle = hiddenWindowHandle;
    …
}

There's two problems here. First of all, data doesn't point to an HWND, it points to an std::atomic<HWND> now, so you already have undefined behavior there. The main problem, and probably the explanation why your original code didn't just happen to work anyways, despite the data race, is that you create a new local HWND called hwHandle. This local variable is initialized with the value of whatever data points to. You then assign your result to that local variable, but never to the actual result variable.

What you want to do is more something along the lines of

unsigned int __stdcall windowsPowerThread(void* data)
{
    …
    HWND hiddenWindowHandle = createHiddenWindow(…);
    *static_cast<std::atomic<HWND>*>(data) = hiddenWindowHandle;
    …
}

You may also want to consider using std::thread instead of the raw CRT functions.

Michael Kenzel
  • 15,508
  • 2
  • 30
  • 39