How to handle a situation that requires to release a lock during a blocking call?
Your use of LockList()
is wrong, and dangerous. As soon as you call UnlockList()
, the TList
is no longer protected, and will be modified as the worker threads remove themselves from the list. That can happen before you have a chance to call WaitForMultipleObjects()
, or worse WHILE setting up the call stack for it.
What you need to do instead is lock the list, copy the handles to a local array, unlock the list, and then wait on the array. DO NOT wait on the TList
itself directly.
procedure WaitForWorkers;var ThreadHandleList: TList; ThreadHandleArr: array of THandle;begin ThreadHandleList := TWorkerThread.WorkerHandleList.LockList; try SetLength(ThreadHandleArr, ThreadHandleList.Count); for I := 0 to ThreadHandleList.Count-1 do ThreadHandleArr[i] := ThreadHandleList[i]; finally TWorkerThread.WorkerHandleList.UnlockList; end; WaitForMultipleObjects(Length(ThreadHandleArr), PWOHandleArray(ThreadHandleArr), True, INFINITE);end;
However, even that has a race condition. Some of the worker threads may have already terminated, and thus destroyed their handles, before WaitForMultipleObjects()
is actually entered. And the remaining threads will destroy their handles WHILE it is running. Either way, it fails. You CANNOT destroy the thread handles while you are actively waiting on them.
FreeOnTerminate=True
can only be used safely for threads that you start and then forget even exist. It is very dangerous to use FreeOnTerminate=True
when you still need to access the threads for any reason (it is especially because of this caveat that TThread.WaitFor()
tends to crash when FreeOnTerminate=True
- the thread handle and even the TThread
object itself gets destroyed while it is still being used!).
You need to re-think your waiting strategy. I can think of a few alternatives:
don't use
WaitForMultipleObjects()
at all. It is safer, but less efficient, to simply re-lock the list periodically and check if it is empty or not:procedure WaitForWorkers;var ThreadHandleList: TList;begin repeat ThreadHandleList := TWorkerThread.WorkerHandleList.LockList; try if ThreadHandleList.Count = 0 then Exit; finally TWorkerThread.WorkerHandleList.UnlockList; end; Sleep(500); until False;end;
Get rid of
WorkerHandleList
altogether and use a semaphore or interlocked counter instead to keep track of how many threads have been created and have not been destroyed yet. Exit the wait when the semaphore/counter indicates that no more threads exist.like Ken B suggested, keep using
WorkerHandleList
but wait on a manual-reset event that gets reset when the first thread is added to the list (do that in the thread constructor, not inExecute()
) and signaled when the last thread is removed from the list (do that in the thread destructor, not inExecute()
orDoTerminate()
).
Assuming a few of things (That only the main thread would start secondary ones, amongst other), the simplest way to fix the issue would be like this :
procedure WaitForWorkers;var ThreadHandleList: TList; iItemCount : Integer;begin repeat ThreadHandleList := TWorkerThread.WorkerHandleList.LockList; try iItemCount := ThreadHandleList.Count finally TWorkerThread.WorkerHandleList.UnlockList; end; if iItemCount = 0 then BREAK; sleep(Whatever_is_suitable); until False;end;
If wasting any cpu cycle, or waiting longer than necessary is not acceptable, you could create an Event and wait on it instead, and have all the thread remove themselves from the list through the same function.
procedure WaitForWorkers;begin Event.WaitFor(INFINITE);end;procedure RemoveHandleFromList(AHandle : THandle);var ThreadHandleList: TList; idx : Integer;begin ThreadHandleList := TWorkerThread.WorkerHandleList.LockList; try idx := ThreadHandleList.IndexOf(Pointer(AHandle)); if idx >= 0 then begin ThreadHandleList.Delete(idx); if ThreadHandleList.Count = 0 then Event.SetEvent; end; finally TWorkerThread.WorkerHandleList.UnlockList; end;end;
In this case, you'd probably want to use a manual reset event and reset it in a "AddHandleToList" procedure.