How to handle a situation that requires to release a lock during a blocking call? How to handle a situation that requires to release a lock during a blocking call? multithreading multithreading

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:

  1. 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;
  2. 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.

  3. 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 in Execute()) and signaled when the last thread is removed from the list (do that in the thread destructor, not in Execute() or DoTerminate()).


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.