Delphi threads deadlock

Question

I am having a problem sometimes with a deadlock when destroying some threads. I've tried to debug the problem but the deadlock never seems to exist when debugging in the IDE, perhaps because of the low speed of the events in the IDE.

The problem:

The main thread creates several threads when the application starts. The threads are always alive and synchronizing with the main thread. No problems at all. The threads are destroyed when the application ends (mainform.onclose) like this:

thread1.terminate;
thread1.waitfor;
thread1.free;

and so on.

But sometimes one of the threads (which logs some string to a memo, using synchronize) will lock the whole application when closing. I suspect that the thread is synchronizing when I call waitform and the harmaggeddon happens, but that's is just a guess because the deadlock never happens when debugging (or I've never been able to reproduce it anyway). Any advice?

Answer 1

Logging messages is just one of those areas where Synchronize() doesn't make any sense at all. You should instead create a log target object, which has a string list, protected by a critical section, and add your log messages to it. Have the main VCL thread remove the log messages from that list, and show them in the log window. This has several advantages:

• You don't need to call Synchronize(), which is just a bad idea. Nice side effect is that your kind of shutdown problems disappear.

• Worker threads can continue with their work without blocking on the main thread event handling, or on other threads trying to log a message.

• Performance increases, as multiple messages can be added to the log window in one go. If you use BeginUpdate() and EndUpdate() this will speed things up.

There are no disadvantages that I can see - the order of log messages is preserved as well.

Edit:

I will add some more information and a bit of code to play with, in order to illustrate that there are much better ways to do what you need to do.

Calling Synchronize() from a different thread than the main application thread in a VCL program will cause the calling thread to block, the passed code to be executed in the context of the VCL thread, and then the calling thread will be unblocked and continue to run. That may have been a good idea in the times of single processor machines, on which only one thread can run at a time anyway, but with multiple processors or cores it's a giant waste and should be avoided at all costs. If you have 8 worker threads on an 8 core machine, having them call Synchronize() will probably limit the throughput to a fraction of what's possible.

Actually, calling Synchronize() was never a good idea, as it can lead to deadlocks. One more convincing reason to not use it, ever.

Using PostMessage() to send the log messages will take care of the deadlock issue, but it has its own problems:

• Each log string will cause a message to be posted and processed, causing much overhead. There is no way to handle several log messages in one go.

• Windows messages can only carry machine-word sized data in parameters. Sending strings is therefore impossible. Sending strings after a typecast to PChar is unsafe, as the string may have been freed by the time the message is processed. Allocating memory in the worker thread and freeing that memory in the VCL thread after the message has been processed is a way out. A way that adds even more overhead.

• The message queues in Windows have a finite size. Posting too many messages can lead to the queue to become full and messages being dropped. That's not a good thing, and together with the previous point it leads to memory leaks.

• All messages in the queue will be processed before any timer or paint messages will be generated. A steady stream of many posted messages can therefore cause the program to become unresponsive.

A data structure that collects log messages could look like this:

type
  TLogTarget = class(TObject)
  private
    fCritSect: TCriticalSection;
    fMsgs: TStrings;
  public
    constructor Create;
    destructor Destroy; override;

    procedure GetLoggedMsgs(AMsgs: TStrings);
    procedure LogMessage(const AMsg: string);
  end;

constructor TLogTarget.Create;
begin
  inherited;
  fCritSect := TCriticalSection.Create;
  fMsgs := TStringList.Create;
end;

destructor TLogTarget.Destroy;
begin
  fMsgs.Free;
  fCritSect.Free;
  inherited;
end;

procedure TLogTarget.GetLoggedMsgs(AMsgs: TStrings);
begin
  if AMsgs <> nil then begin
    fCritSect.Enter;
    try
      AMsgs.Assign(fMsgs);
      fMsgs.Clear;
    finally
      fCritSect.Leave;
    end;
  end;
end;

procedure TLogTarget.LogMessage(const AMsg: string);
begin
  fCritSect.Enter;
  try
    fMsgs.Add(AMsg);
  finally
    fCritSect.Leave;
  end;
end;

Many threads can call LogMessage() concurrently, entering the critical section will serialize access to the list, and after adding their message the threads can continue with their work.

That leaves the question how the VCL thread knows when to call GetLoggedMsgs() to remove the messages from the object and add them to the window. A poor man's version would be to have a timer and poll. A better way would be to call PostMessage() when a log message is added:

procedure TLogTarget.LogMessage(const AMsg: string);
begin
  fCritSect.Enter;
  try
    fMsgs.Add(AMsg);
    PostMessage(fNotificationHandle, WM_USER, 0, 0);
  finally
    fCritSect.Leave;
  end;
end;

This still has the problem with too many posted messages. A message needs only be posted when the previous one has been processed:

procedure TLogTarget.LogMessage(const AMsg: string);
begin
  fCritSect.Enter;
  try
    fMsgs.Add(AMsg);
    if InterlockedExchange(fMessagePosted, 1) = 0 then
      PostMessage(fNotificationHandle, WM_USER, 0, 0);
  finally
    fCritSect.Leave;
  end;
end;

That still can be improved, though. Using a timer solves the problem of the posted messages filling up the queue. The following is a small class that implements this:

type
  TMainThreadNotification = class(TObject)
  private
    fNotificationMsg: Cardinal;
    fNotificationRequest: integer;
    fNotificationWnd: HWND;
    fOnNotify: TNotifyEvent;
    procedure DoNotify;
    procedure NotificationWndMethod(var AMsg: TMessage);
  public
    constructor Create;
    destructor Destroy; override;

    procedure RequestNotification;
  public
    property OnNotify: TNotifyEvent read fOnNotify write fOnNotify;
  end;

constructor TMainThreadNotification.Create;
begin
  inherited Create;
  fNotificationMsg := RegisterWindowMessage('thrd_notification_msg');
  fNotificationRequest := -1;
  fNotificationWnd := AllocateHWnd(NotificationWndMethod);
end;

destructor TMainThreadNotification.Destroy;
begin
  if IsWindow(fNotificationWnd) then
    DeallocateHWnd(fNotificationWnd);
  inherited Destroy;
end;

procedure TMainThreadNotification.DoNotify;
begin
  if Assigned(fOnNotify) then
    fOnNotify(Self);
end;

procedure TMainThreadNotification.NotificationWndMethod(var AMsg: TMessage);
begin
  if AMsg.Msg = fNotificationMsg then begin
    SetTimer(fNotificationWnd, 42, 10, nil);
    // set to 0, so no new message will be posted
    InterlockedExchange(fNotificationRequest, 0);
    DoNotify;
    AMsg.Result := 1;
  end else if AMsg.Msg = WM_TIMER then begin
    if InterlockedExchange(fNotificationRequest, 0) = 0 then begin
      // set to -1, so new message can be posted
      InterlockedExchange(fNotificationRequest, -1);
      // and kill timer
      KillTimer(fNotificationWnd, 42);
    end else begin
      // new notifications have been requested - keep timer enabled
      DoNotify;
    end;
    AMsg.Result := 1;
  end else begin
    with AMsg do
      Result := DefWindowProc(fNotificationWnd, Msg, WParam, LParam);
  end;
end;

procedure TMainThreadNotification.RequestNotification;
begin
  if IsWindow(fNotificationWnd) then begin
    if InterlockedIncrement(fNotificationRequest) = 0 then
     PostMessage(fNotificationWnd, fNotificationMsg, 0, 0);
  end;
end;

An instance of the class can be added to TLogTarget, to call a notification event in the main thread, but at most a few dozen times per second.

Answer 2

Consider replacing Synchronize with a call to PostMessage and handle this message in the form to add a log message to the memo. Something along the lines of: (take it as pseudo-code)

WM_LOG = WM_USER + 1;
...
MyForm = class (TForm)
  procedure LogHandler (var Msg : Tmessage); message WM_LOG;
end;
...
PostMessage (Application.MainForm.Handle, WM_LOG, 0, PChar (LogStr));

That avoids all the deadlock problems of two threads waiting for each other.

EDIT (Thanks to Serg for the hint): Note that passing the string in the described way is not safe since the string may be destroyed before the VCL thread uses it. As I mentioned - this was only intended to be pseudocode.