Multithreaded file write via an existing object

Question

Application Description:

I have an application that allows a user to run multiple concurrent queries via threads (up to 100 at once).

I have a class that I use for logging errors. If an error occurs in the application, I create an instance of the class and call a procedure to write the error to a log file.

Question:

I need to make the error logging code thread safe. I've noticed that if a lot of threads are running at the same time and generating the same error (e.g. cannot connect to database), I'm getting i/o error 32 (caused by the application attempting to write to a file that's already open).

As a quick and dirty fix, I've put the code that writes to file in a try... except block inside a repeat loop. If there is an exception (e.g. the file has already been opened by another instance of the class, kicked off by another thread), then it sets a flag to "false". The loop continues to execute until the flag is "true" (i.e. no error writing to file), as follows:

procedure TErrorLogging.logError(error: string);
var
     f: textfile;
     ok: boolean;
begin
     repeat
          ok := true;
          try
               assignfile(f, fLogFilename);
               if fileExists(fLogFilename) then append(f) else rewrite(f);
               writeln(f, error);
               closefile(f);
          except
               ok := false;
          end;
     until ok;
end;

I'm aware that the correct way to protect blocks of code is by using Critical Sections, but I'm not sure how I'd implement that, given that there are a number of different threads that use the logging class, and each instance of the thread has its own instance of the logging class that it uses to write to file (so they're not all just synchronizing against the same block of code).

The options, as I can see them:

1. Use the code as above. Are there any issues with leaving this code as it is? It's a quick and dirty fix, but it works.
2. Use a global TCriticalSection (how?).
3. Use a single procedure somewhere that creates an instance of the logging class, which the threads will synchronize against (which defeats the object of having a logging class, I suppose).

Answer 1

Creating instance of a logging class whenever you want to append log entry is wrong as well as opening and closing a log file over and over again. I would personally use one instance of a class which internally uses a string list and whose basic methods are thread safe. Something like this:

type
  TErrorLog = class
  private
    FList: TStringList;
    FLock: TRTLCriticalSection;
  public
    constructor Create;
    destructor Destroy; override;
    procedure Clear;
    procedure Add(const ErrorText: string);
    procedure SaveToFile(const FileName: string);
  end;

implementation

{ TErrorLog }

constructor TErrorLog.Create;
begin
  inherited Create;
  InitializeCriticalSection(FLock);
  FList := TStringList.Create;
end;

destructor TErrorLog.Destroy;
begin
  EnterCriticalSection(FLock);
  try
    FList.Free;
    inherited Destroy;
  finally
    LeaveCriticalSection(FLock);
    DeleteCriticalSection(FLock);
  end;
end;

procedure TErrorLog.Clear;
begin
  EnterCriticalSection(FLock);
  try
    FList.Clear;
  finally
    LeaveCriticalSection(FLock);
  end;
end;

procedure TErrorLog.Add(const ErrorText: string);
begin
  EnterCriticalSection(FLock);
  try
    FList.Add(ErrorText);
  finally
    LeaveCriticalSection(FLock);
  end;
end;

procedure TErrorLog.SaveToFile(const FileName: string);
begin
  EnterCriticalSection(FLock);
  try
    FList.SaveToFile(FileName);
  finally
    LeaveCriticalSection(FLock);
  end;
end;

Answer 2

Not knowing Delphi, as a general design rule (if possible), I would have your logError function insert into a thread safe Array, ArrayList, Queue object or such that you have available, and then have it write to the file in the background, perhaps every 5-10 seconds or so. This should not only take care of the i/o problem, but should also scale to thousands of writes per second in case you want to log other events for debugging or such.