After all this years AllocateHWnd is still not thread safe
After all this years AllocateHWnd is still not thread safe
http://qc.embarcadero.com/wc/qcmain.aspx?d=47559
http://qc.embarcadero.com/wc/qcmain.aspx?d=47559
http://qc.embarcadero.com/wc/qcmain.aspx?d=47559
http://qc.embarcadero.com/wc/qcmain.aspx?d=47559
Oh crap! That can explain some of our sporadic mysterious message issues! Edit: On code inspection - maybe not... unless it is frequently used in other libs.
ReplyDeleteUntil it is fixed, you can use DSiAllocateHWnd from DSiWin32
ReplyDeleteMarkus Joos You can use something else if you know there is a problem in the first place.
ReplyDeleteLars Fosdal MakeObjectInstance and FreeObjectInstance are the culprit here and they are used much more all around than AllocateHWnd/DeallocateHWnd.
ReplyDeleteDalija Prasnikar
ReplyDeleteYou are absolutely correct. Therefore thanks for posting this here, so that more people are aware of it.
Ah, I missed that detail. No MakeObjectInstance in our code, and the Delphi libs mainly appear to use it in VCL, hence largely from the main thread.
ReplyDeleteLars Fosdal But if your call to AllocateHWnd in thread triggers MakeObjectInstance at the same time when some VCL code does you are toast.
ReplyDeleteYou don't need to have collision between two secondary threads, all that is needed is that one secondary thread collides with main one.
Based on a quick inspection, it seems that we largely use allocatehwnd in the main thread on startup, and pass the pre-allocated handles to the threads at create time.
ReplyDeleteLars Fosdal you are safe then :)
ReplyDeleteI certainly hope so - but I just replaced all calls to AllocatehWnd to a shiny new ThreadSafeAllocatehWnd, just in case :)
ReplyDeleteIt will never be fixed. It's by design.
ReplyDeleteLars Fosdal You are not handling thread/window affinity correctly if you do that. Remember that when you send a message to a window, it is executed on that window's thread. And the thread associated with a window is the thread that created the window.
ReplyDeletehttp://stackoverflow.com/questions/8820294/how-can-i-make-allocatehwnd-threadsafe
ReplyDeleteDavid Heffernan - That is the point, isn't it. The background threads passes messages to the foreground thread, which process them.
ReplyDeleteSo long as you call AllocateHWnd in the main thread, it's all good. Sending messages to that window handle from another thread is fine.
ReplyDeleteDavid Heffernan Not really by design, but certainly a very delicate issue. Any extra code will get executed a lot of time, mostly with no need. So it is a tradeoff. We are still considering working on it, though.
ReplyDeleteMarco Cantù By design means, to me, that the code is behaving as intended. And I think it is. I'm happy with the design. I would not change it. Certainly you should not change MakeObjectInstance. Making that threadsafe would have perf implications. You might contemplate an AllocateHWnd that was not built on top of MakeObjectInstance. However, it is very easy to work around, and rare to need to do so. And when one needs to do so, it's easy to wrap it up. So I don't feel too strongly that you need to change anything.
ReplyDeleteDavid Heffernan I mostly agree. But as the poll will likely suggest, many users get in trouble not understanding the issue, and we have many bus logged that are ultimately tied to this. Which is why we are considering a low-level change. Concern is this is misused, and fixing it will just defer the error to a alter API call from the thread...
ReplyDelete+Marco Sometimes you as the vendor need to do what you know to be right, even if less adept users don't understand. You can close the bug reports as "as designed".
ReplyDeletePlease don't introduce overhead to everyone's programs just to let a minority create timer components in threads.
A threadsafe AllocateHWnd, perhaps with a different name would be fine. But please don't make everyone pay.
David Heffernan That's what documentation is for. Used to be for.
ReplyDeleteMarco Cantù AllocateHWnd/DeallocateHWnd are not mission critical methods that will be executed in tight loops millions of times. IMO providing their thread safety cannot have significant performance impacts.
ReplyDeleteI would be even inclined to say that providing thread safe counterparts would be enough, so that people do not have to rich out to other sources for thread safe means of allocating window handles. But frankly, that would be my second choice only and only if you can really prove that thread safety in those methods would have performance impact on some real life code (not an easy task, I know).
David Heffernan Am I missing some quotes in your QC comment or is that another David Heffernan:
ReplyDelete"Oh, come on guys sort this one out please! All you need to do is stuff a critical section around the bodies of MakeObjectInstance and FreeObjectInstance. There's no performance implications. Otherwise how can we use TTimer in a non-UI thread?"
Dalija Prasnikar That's another David Heffernan. From a different time. This one, in 2015, takes a different viewpoint.
ReplyDeleteDavid Heffernan Please remember that David from time to time ;-)
ReplyDeleteDalija Prasnikar I don't disagree. The creation of a Windows handle at the OS level is probably an order of magnitude more expensive in terms of time than a lock. Still, each instruction adds up.
ReplyDeleteMy take is there are huge misconceptions in multi-threading. Most platforms libraries have very limited support for threads managing system objects, message queues, and the like. The only really safe way is to do platform calls that use platform resources (graphical, windows, controls, etc) only in the main thread. It is not a VCL issue, it is mostly a Windows issue.
Still, if we can handle a few common scenarios and have a working solution for those, it will help quite a few developers, even if they don't fully understand what they are doing.
Marco Cantù Using window handle in a thread is not uncommon multi-threaded scenario. Windows is fully capable of handling that. It is also not related to VCL and GUI non-thread safety.
ReplyDeleteAllocateHWnd and DeallocateHWnd are located in System.Classes and since Windows can use window handles in threads, it is not unreasonable to assume that many will automatically think that those methods are thread safe even if they know that VCL and GUI is not thread safe.
Another problem with issue like this is that they are extremely hard to find and track down. You may think that your app is running fine when in reality it is not. Anything Delphi can provide to avoid issues in that regard is welcomed.
"Using window handle in a thread is not uncommon multi-threaded scenario. Windows is fully capable of handling that." I disagree. A large number of Windows APIs, including those that involve messages, might get you in trouble. Also the issue here is not using, but creating handles in a thread, right?
ReplyDelete+Marco Windows is threadsafe, so long as you follow the rules. Windows have thread affinity. Create a handle in a thread, service it there, no problems. The system is designed to support that.
ReplyDeleteHowever, we don't really expect to see visible windows in threads, or user input. One expects to see message only windows in threads.
I disagree with +Dalija. Message only windows in threads aren't common, especially not for non-experts. I do it in one place only in my app, but I could easily do it without a window handle.
I honestly now believe that there's no need for any change. Other than perhaps better documentation.
+Dalija "It is also not related to VCL and GUI non-thread safety."
ReplyDeleteI don't agree. The VCL is a wrapper of Win32. As such it reflects the Win32 threading rules. Hence VCL design mandating use from main thread only. That was a design choice across the entire VCL, reflecting the Win32 threading model. With that in place, it is logical that MakeObjectInstance is coded the way it is.
+Marco You could just raise an exception in MakeObjectInstance if it's not called from the main thread. That would address the misuse concerns.
ReplyDeleteRaising an exception if not running in main thread, would break current code.
ReplyDeleteHow large would the execution cost be if you added a critical section to the currently vulnerable code? It doesn't seem to be time critical, nor does it seem to be prone to concurrency issues like deadlocking?
Simon Stuart TTimer is a GUI timer and serves that purpose well. Other problems require other solutions.
ReplyDeleteLars Fosdal That code is already broken though. Better to know about it sooner rather than later.
ReplyDeleteMarco Cantù If you want to use that window handle to process messages inside thread then you have to create it inside thread.
ReplyDeleteOk, maybe doing that is more uncommon than I think, but currently Delphi does not provide "out of the box" thread safe way of allocating window handle in a thread.
You may say that this is something everyone should do for themselves, but again AllocateHWnd is part of System.Classes and not VCL units that are not thread safe.
Simon Stuart Which means that your opinion isn't important, because you aren't a user. I don't mean that in a rude way. I personally don't use databases, but I wouldn't suggest that Embarcadero could just ignore users that do.
ReplyDeleteDavid Heffernan Yes, VCL is not thread safe, but AllocateHWnd is in System.Classes and Windows, while not thread safe in some areas, are capable of having window handle in a thread and serving its message loop safely.
ReplyDeleteFYI, I have never used timer in thread.
Simon Stuart Timers are not only used for animations...
ReplyDeleteI've touched timers from a thread (disable/enable), but they were owned by the main thread. Handy if you want to trigger a refresh, but need to avoid refresh storms.
ReplyDeleteDalija Prasnikar I think it should be clear now that I am aware of that fact. I'd be alright with a new AllocateHWnd that was thread safe. Or a new function named ThreadsafeAllocateHWnd perhaps. I do feel that this isn't the biggest problem ever since anyone capable of servicing a window in a thread, is capable of creating a window.
ReplyDeleteWhat I don't want to happen is there to be added synchronization that everybody pays for. As a library designer, one should generally not force synchronization choices onto the consumer of the library.
David Heffernan I made rough timing experiment with allocating/deallocating window handle (10000 times) and adding lock around those calls (just to measure impact of lock). Difference is almost non-existent. Allocating window handles is so unpredictable that I had runs with locks that were running in half the time of some runs without locks.
ReplyDeleteI think it is safe to say that thread safe version of AllocateHWnd should not have any performance impacts on any existing code.
Dalija Prasnikar So long as there's no discernible cost that would be fine by me.
ReplyDeleteDavid Heffernan There should be any costs. Of course, there are many ways to skin the cat, but if done properly thread safe version of AlllocateHWnd would be cost free for all.
ReplyDeleteI am not implying in any way how this should be achieved, and I am not proposing that it should be done by making MakeObjectInstance thread safe. I am leaving decisions like that to those that know RTL better than I do.
Adding a runtime check to a library at no runtime cost is an illusion. It could be added with very limited runtime cost when not needed, and a higher runtime cost when needed. But unless you don't write any code, you'll have to run it...
ReplyDelete