views:

332

answers:

2

I have just written my own logging framework (very lightweight, no need for a big logging framework). It consists of an interface ILogger and a number of classes implementing that interface. The one I have a question about is TGUILogger which takes a TStrings as the logging target and synchronizes the logging with the main thread so that the Items member of a listbox can be used as the target.

type
  ILogger = Interface (IInterface)
    procedure Log (const LogString : String; LogLevel : TLogLevel);
    procedure SetLoggingLevel (LogLevel : TLogLevel);
  end;

type    
  TGUILogger = class (TInterfacedObject, ILogger)
  public
    constructor Create (Target : TStrings);
    procedure Log (const LogString : String; LogLevel : TLogLevel);
    procedure SetLoggingLevel (LogLevel : TLogLevel);
  private
    procedure PerformLogging;
  end;

procedure TGUILogger.Log (const LogString : String; LogLevel : TLogLevel);
begin
  TMonitor.Enter (Self);
  try
    FLogString := GetDateTimeString + ' ' + LogString;
    TThread.Synchronize (TThread.CurrentThread, PerformLogging);
  finally
    TMonitor.Exit (Self);
  end;
end;

procedure TGUILogger.PerformLogging;
begin
  FTarget.Add (FLogString);
end;

The logging works, but the application does not close properly. It seems to hang in the Classes unit. The stack trace:

System.Halt0, System.FinalizeUnits, Classes.Finalization, Classes.FreeExternalThreads, System.TObject.Free, Classes.TThread.Destroy, Classes.TThread.RemoveQueuedEvents

What am I doing wrong here?

EDIT: I just found the following hint in the Delphi help for TThread.StaticSynchronize

Warning: Do not call StaticSynchronize from within the main thread. This can cause 
an infinite loop.

This could be exactly my problem since some logging request come from the main thread. How can I solve this?

+2  A: 

If you don't find any simpler way, you could try doing this:

At program initialization, (from the main thread,) have your logging subsystem call the Windows API function GetCurrentThreadID and store the result in a variable. (EDIT: the MainThreadID variable in the System unit, gets initialized this way automatically for you at startup. Thanks, mghie.) When a logging request comes in after that, call GetCurrentThreadID again, and only synchronize if it's coming from a different thread.

There are other tricks that don't involve the Windows API, but they end up being more complicated, especially if you have a bunch of different custom TThread descendants. The basic principle is the same, though: Verify whether or not you're in the main thread before you decide whether or not to call StaticSynchronize.

Mason Wheeler
What is wrong with using MainThreadId and TThread.ThreadId?
mghie
Ah, that's the "any simpler way". I figured there had to be a built-in way to get the ID of the main thread, but I didn't know about MainThreadID. I'll edit my response. The problem with TThread.ThreadID, though, is that you require a reference to the current thread to use it.
Mason Wheeler
Ah yes, that is indeed a problem. But the thread ID values are only needed for comparison here anyway, because the Delphi RTL is missing a IsMainThread() function or something similar. That would have been a useful addition... like 13 years ago? With Delphi 2?
mghie
Yeah. And personally I'm surprised there isn't an Application.ThreadID property, or even an Application.MainThread: TThread property. Those would both be useful in certain cases...
Mason Wheeler
I just realized that TThread.ThreadId is useless here anyway, because there is no guarantee that the thread method is called in the correct thread context - it could be called from any thread. What would be needed: a TThread class method returning the "current" TThread instance (or nil).
mghie
Re Application.ThreadId: MainThreadId will also work in console applications.
mghie
TThread.ThreadID: Yeah, that's the "more complicated trick" I was talking about. It involves a global threadvar: CurrentThread: TThread. All your threads need to set CurrentThread := self; in their constructors, and and for the main thread, it'll be left nil.
Mason Wheeler
Application.ThreadID: Sure, but it would still be very useful for VCL apps, where you just sort of expect that Application will contain all the pertinent data about the state of your application.
Mason Wheeler
+7  A: 

If you compare the CurrentThreadID with MainThreadID then you can choose to synchronize or not.

Personally, I chose to have the GUI ask the log system for the latest info, rather than have threads pause. Otherwise your logging interferes with the fast operation of the thread which defeats the purpose of having it.

mj2008
+1, unfortunately I can't vote more than once. Both paragraphs deserve their up votes, the second much more than one. What you don't want in your logging framework is slow down, thread serialization and forced context switches. Using Synchronize() gives you all three of them. What a nightmare.
mghie
+1 good answer. To your second point and why I chose to implement it the way I did: I wanted all the logging in one place so that I can have GUI loggers, file loggers, email loggers etc. without changing the GUI. I can live with not so great performance if the design is clean and easily changeable.
Smasher