TMonitor synchronization / Application.ProcessMessages

Question

I'm back with another question concerning threads and synchronization. Imagine a server application that has to perform a lengthy operation and the client wants his GUI to remain responsive while he waits for the server's response. I thought of the following pattern:

TMonitor.Enter (FTCPClient);
try
  WorkerThread := TWorkerThread.Create (SomeLengthyServerOperation);
  while (not WorkerThread.Ready) do
    Application.ProcessMessages;
  DoSometingWithResults (WorkerThread.Result);
  WorkerThread.Free;      
finally
  TMonitor.Exit (FTCPClient);
end;

WorkerThread is a simple class derived from TThread that executes the function passed to its constructor and then terminates (with Ready=True and the result in Result). The presented code is executed whenever a button is clicked.

Now to my question: If I click the button twice very fast, I get some strange errors that look a lot like the communication beetween server and client is somehow mengled, which I wanted to avoid by locking the FTCPClient object. In what thread are the event handlers after Application.ProcessMessages executed? Is the lock of TMonitor per thread? Does this mean that the lock does not work if I use Application.ProcessMessages?

I can't explain it better at the moment. I hope someone gets my point. If not, feel free to ask questions.

EDIT: For the Disabling and Enabling of the button: I don't know anything about the client code. Could be a button event handler, could be something else. Basically I want to hide the locking from the client code.

Answer 1

TMonitor only blocks a different thread from acquiring the lock. What's happening is this: by processing messages from within the lock, you're getting back into the same function in the same thread, which is causing a recursive acquisition of the lock. Your code is then creating a new worker thread, and starting the cycle all over. You could disable the button so that you cannot click it again until the worker thread completes. Be sure you disable the button before you start processing the messages and use another try..finally block to ensure it gets re-enabled. Depending on how the rest of your code is arranged, you may not even need the lock.

Answer 2

Some comments:

1. Your WorkerThread sounds like you just reimplemented AsyncCalls. Using a tried and tested implementation is probably better than writing your own (unless you do it for the learning effect or because you have very special requirements). Please have a look at both AsyncCalls and the OmniThreadLibrary.

2. Locking should happen only as short as possible, so wrapping your whole reaction to a button click in Monitor.Enter() and Monitor.Exit() seems wrong. It is also not really understandable what purpose this serves.

3. Calling Application.ProcessMessages() while a lock is held can introduce all kinds of nasty surprises. If you need to keep your code from being reentered it is generally best to disable all the UI elements as the first thing a OnClick handler does, and reenable them when the handler is finished. Do also note that locks can be entered multiple times from the same thread, they only achieve exclusive access from multiple threads.

4. All of the VCL executes in the main GUI thread, so locking is only ever necessary when you call the same code from background threads.

5. If you have a look at this code you will see that you could achieve the same thing by doing your work in the GUI thread instead of spawning a worker thread.

I have posted this link several times on StackOverflow already, but please consider following the advice in this list posting for some things to keep in mind when doing multi-threaded programming. It has some good advice especially regarding the fifth point.

Edit: It's hard to say exactly, but your code should probably be something like this:

procedure TForm1.ActionStartExecute(Sender: TObject);
begin
  ActionStart.Enabled := FALSE;
  fWorkerThread := TWorkerThread.Create (Handle, SomeLengthyServerOperation);
end;

procedure TForm1.ActionStartUpdate(Sender: TObject);
begin
  ActionStart.Enabled := fWorkerThread = nil;
end;

procedure TForm1.WMThreadFinished(var AMsg: TWMThreadFinishedMsg);
begin
  // process results
  fWorkerThread := nil;
end;

TWorkerThread frees itself, but as the last action it posts a message to the form (that's why it gets the window handle as a parameter). In the handler for this message you set fWorkerThread to nil, so that the action is re-enabled in the next idle loop. Using a TAction means that you need not care what UI element originated the thread creation - they will all be disabled when the thread is created, and will be re-enabled when the thread has finished. No locking is necessary, and no new thread can be created while a thread is active.