## Summary The Win32 implementation of TThread.WaitFor (`rtl/win/tthread.inc`) only handles 3 outcomes from MsgWaitForMultipleObjects: CheckSynchronize, Thread terminated, Message arrived. If the thread handle is closed while this loop still runs (which can happen if the thread has FreeOnTerminate set and frees itself from the ThreadProc while the main thread handles a synchronize call), MsgWaitForMultipleObjects returns WAIT_FAILED which is not handled, therefore the loop is never exited. ## System Information - **Operating system:** Windows 10 - **Processor architecture:** x86 and x86-64 - **Compiler version:** trunk (0420b033390aa01745306fb25f2b804f2f52c4fd) ## Steps to reproduce Exact reproducer is not easy as it needs the main thread to do something during the time the thread takes to a) return from .Execute b) call .Free c) finish its ThreadProc while missing the short time between a) and b) where the thread would be signaled, so just a sketch. The dirwatch class in threaded mode also can trigger it quite well due to its DoIdle callbacks. ```pas thr:= TThread.Create(..); thr.FreeOnTerminate:= true; // ... thr.WaitFor; ``` ## What is the current bug behavior? The loop in WaitFor is never left, but keeps running and would execute Synchronize callbacks (=livelock). ## What is the expected (correct) behavior? This is actually more difficult for me to specify. Delphi throws an exception if the thread goes away in the loop, but I don't think that is either correct or helpful. IMO the thread being closed a perfectly reasonable thing to happen, but the ExitCode in that case is not retrievable from the invalid handle anymore, so what should WaitFor return? ## Possible fixes A stupid simple fix that I have tried that and works, but see the caveat on the return code above: ```diff diff --git a/rtl/win/tthread.inc b/rtl/win/tthread.inc index 1133175608..f5d08f8220 100644 --- a/rtl/win/tthread.inc +++ b/rtl/win/tthread.inc @@ -104,6 +104,14 @@ function TThread.WaitFor: Integer; while true do begin case MsgWaitForMultipleObjects(2, WaitHandles, False, INFINITE, QS_SENDMESSAGE) of + WAIT_FAILED: + begin + Result:= GetLastError; + // ERROR_INVALID_HANDLE indicates that the thread closed while we are waiting + // for it. This can happen when FreeOnTerminate is set and the ThreadProc frees + // the thread while the MainThread was handling a Synchronize callback. + break; + end; WAIT_OBJECT_0: break; WAIT_OBJECT_0+1: ``` An alternative could be to duplicate the thread handle locally to keep it alive and then wait for *that* to become signaled. Seems to work as well, but should be checked for if there are any issues with leaks here. ```diff diff --git a/rtl/win/tthread.inc b/rtl/win/tthread.inc index 1133175608..872663b49c 100644 --- a/rtl/win/tthread.inc +++ b/rtl/win/tthread.inc @@ -95,11 +95,14 @@ procedure TThread.Resume; function TThread.WaitFor: Integer; var Msg: TMsg; + lHandle: THandle; WaitHandles : array[0..1] of THandle; begin if GetCurrentThreadID = MainThreadID then begin - WaitHandles[0]:=FHandle; + // Duplicate the handle to ensure it stays valid through any FreeOnTerminate actions + DuplicateHandle(GetCurrentProcess, FHandle, GetCurrentProcess, @lHandle, 0, false, DUPLICATE_SAME_ACCESS); + WaitHandles[0]:=lHandle; WaitHandles[1]:=THandle(SynchronizeTimeoutEvent); while true do begin @@ -112,8 +115,13 @@ function TThread.WaitFor: Integer; PeekMessage(Msg, 0, 0, 0, PM_NOREMOVE) end; end; + GetExitCodeThread(lHandle, DWord(Result)); + // Close our duplicate. If the thread is already closed, this will finally free it. + CloseHandle(lHandle); end else - WaitForSingleObject(ulong(FHandle), INFINITE); - GetExitCodeThread(FHandle, DWord(Result)); + begin + WaitForSingleObject(ulong(FHandle), INFINITE); + GetExitCodeThread(FHandle, DWord(Result)); + end; end; ```