Ensure all TThread.Queue methods complete before thread self-destructs

Question

I have discovered that if a method queued with TThread.Queue calls a method that invokes TApplication.WndProc (e.g. ShowMessage) then subsequent queued methods are allowed to run before the original method has completed. Worse still, they don't seem to be invoked in FIFO order.

[Edit: Actually they do start in FIFO order. With ShowMessage it looks like the later one ran first because there is a call to CheckSynchronize before the dialog appears. This unqueues the next method and runs it, not returning until the latter method has completed. Only then does the dialog appear.]

I'm trying to ensure that all methods queued from the worker thread to run in the VCL thread run in strict FIFO order, and that they all complete before the worker thread is destroyed.

My other constraint is that I am trying to maintain strict separation of the GUI from the business logic. The thread in this case is part of the business logic layer so I can't use PostMessage from an OnTerminate handler to arrange for the thread to be destroyed (as recommended by a number of contributors elsewhere). So I'm setting FreeOnTerminate := True in a final queued method just before TThread.Execute exits. (Hence the need for them to execute in strict FIFO order.)

This is how my TThread.Execute method ends:

finally
  // Queue a final method to execute in the main thread that will set an event
  // allowing this thread to exit. This ensures that this thread can't exit
  // until all of the queued procedures have run.
  Queue(
    procedure
    begin
      if Assigned(fOnComplete) then
      begin
        fOnComplete(Self);
        // Handler sets fWorker.FreeOnTerminate := True and fWorker := nil
      end;
      SetEvent(fCanExit);
    end);
  WaitForSingleObject(fCanExit, INFINITE);
end;

but as I said this doesn't work because this queued method executes before some of the earlier queued methods.

Can anyone suggest a simple and clean way to make this work, or a simple and clean alternative?

[The only idea I've come up with so far that maintains separation of concerns and modularity is to give my TThread subclass a WndProc of its own. Then I can use PostMessage directly to this WndProc instead of the main form's. But I'm hoping for something a bit more light-weight.]

---

Thanks for the answers and comments so far. I now understand that my code above with a queued SetEvent and WaitForSingleObject is functionally equivalent to calling Synchronize at the end instead of Queue because Queue and Synchronize share the same queue. I tried Synchronize first and it failed for the same reason as the code above fails - the earlier queued methods invoke message handling so the final Synchronize method runs before the earlier queued methods have completed.

So I'm still stuck with the original problem, which now boils down to: Can I cleanly ensure that all of the queued methods have completed before the worker thread is freed, and can I cleanly free the worker thread without using PostMessage, which requires a window handle to post to (that my business layer doesn't have access to).

I've also updated the title better to reflect the original problem, although I'd be happy for an alternative solution that doesn't use TThread.Queue if appropriate. If someone can think up a better title then please edit it.

---

Another update: This answer by David Heffernan suggests using PostMessage with a special AllocateHWnd in the general case if TThread.Queue isn't available or suitable. Significantly, it's never safe to use PostMessage to the main form because the window can be spontaneously recreated changing its handle, which would cause all subsequent messages to the old handle to be lost. This makes a strong argument for me adopting this particular solution, since there's no additional overhead to creating a hidden window in my case since any application using PostMessage should do this - i.e. my separation of concerns argument is irrelevant.

Answer 1

I fixed this problem by adding a call to Synchronize() at the end of my Execute() method. This forces the thread to wait till all of the calls added with Queue() are completed on the main thread before the call added with Synchronize() can be called.

TMyThread = class (TThread)
private
  procedure QueueMethod;
  procedure DummySync;
protected
  procedure Execute; override;
end;

procedure TMyThread.QueueMethod;
begin
  // Do something on the main thread 
  UpdateSomething;
end;

procedure TMyThread.DummySync;
begin
  // You don't need to do anything here. It's just used
  // as a fence to stop the thread ending before all the 
  // Queued messages are processed.
end;

procedure TMyThread.Execute;
begin
  while SomeCondition do 
  begin
     // Some process

     Queue(QueueMethod);
  end;
  Synchronize(DummySync);
end;

Answer 2

A few thoughts:

1. FreeOnTerminate is not the end of the world if you want your thread to delete itself.
2. Semaphores let you maintain a count should you feel the need, there are such constructs.
3. There's nothing to stop you writing or using your own queueing primitives and AllocateHWnd if you want some fine grained control.

Answer 3

This is the solution I finally adopted.

I used a Delphi TCountdownEvent to track the number of outstanding queued methods from my thread, incrementing the count just before queuing a method, and decrementing it as the final act of the queued method.

Just before my override of TThread.Execute returns, it waits for the TCountdownEvent object to be signalled, i.e. when the count reaches zero. This is the crucial step that guarantees that all of the queued methods have completed before Execute returns.

Once all of the queued methods are complete, it calls Synchronize with an OnComplete handler - thanks to Remy for pointing out that this is equivalent to but simpler than my original code that used Queue and WaitForSingleObject. (OnComplete is like OnTerminate, but called before Execute returns so that the handler can modify FreeOnTerminate.)

The only wrinkle is that TCountdownEvent.AddCount works only if the count is already greater than zero. So I wrote a class helper to implement ForceAddCount:

procedure TCountdownEventHelper.ForceAddCount(aCount: Integer);
begin
  if not TryAddCount(aCount) then
  begin
    Reset(aCount);
  end;
end;

Normally this would be risky, but in my case we know that by the time the thread starts waiting for number of queued methods outstanding to reach zero no more methods can be queued (so from this point once the count hits zero it will stay at zero).

This doesn't completely solve the problem of queued methods that handle messages, in that individual queued methods could still appear to run out of order. But I do now have the guarantee that all queued methods run asynchronously but will have completed before the thread exits. This was the primary goal, because it allows the thread to clean itself up without the risk of losing queued methods.

Answer 4

TThread.Queue() is a FIFO queue. In fact, it shares the same queue that Thread.Sychronize() uses. But you are correct that message handling does cause queued methods to execute. This is because TApplication.Idle() calls CheckSynchronize() whenever the message queue goes idle after processing new messages. So if a queued/synched method invokes message processing, that can allow other queued/synched methods to being running even if the earlier method is still running.

If you want to ensure a queue method is called before the thread terminates, you should be using Synchronize() instead of Queue(), or use the OnTerminate event instead (which is triggered by Synchronize()). What you are doing in your finally block is effectively the same as what the OnTerminate event already does natively.

Setting FreeOnTerminate := True in a queued method is asking for a memory leak. FreeOnTerminate is evaluated immediately when Execute() exits, before DoTerminate() is called to trigger the OnTerminate event (which is an oversight in my opinion, as evaluating it that early prevents OnTerminate from deciding at termination time whether a thread should free itself or not after OnTerminate exits). So if the queued method runs after Execute() has exited, there is no guarantee that FreeOnTerminate will be set in time. Waiting for a queued method to finish before returning control to the thread is exactly what Synchronize() is meant for. Synchronize() is synchronous, it waits for the method to exit. Queue() is asynchronous, it does not wait at all.