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:

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.
Use a global TCriticalSection (how?).
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).

"Are there any issues with leaving this code as it is?" Yes, it makes you feel dirty. "Use a critical section (how)?" There's nothing to it. Create a global variable to hold your lock and then use it to serialize access. Anyway, I don't understand why you need more than one instance of this logging class. — David Heffernan
– David Heffernan, Commented Mar 26, 2013 at 17:58
There are open source logging frameworks available for Delphi like Log4D which include thread-safe log file writers (called 'appenders'). You could use the TLogFileAppender code as a starting point. — mjn
– mjn, Commented Mar 27, 2013 at 10:35

TLama · Accepted Answer · 2013-03-26 18:09:30Z

6

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;

answered Mar 26, 2013 at 18:09

TLama

77k17 gold badges227 silver badges419 bronze badges

Sign up to request clarification or add additional context in comments.

3 Comments

bummi Over a year ago

+1 In case of long running programs or much information to log (mmhh most cases) I'd perfer a filestream for logging

LU RD Over a year ago

Somehow I would expect a TErrorLog class to handle the SaveToFile without an explicit call.

TLama Over a year ago

Both above comments might be merged into one. Yes, it might be good either to use a file stream, or implement some sort of batch append saving when a certain count of entries will be in a log for instance. @LU RD, you're right, but mainly I was going to show how to use a critical section here.

Stanley · Accepted Answer · 2013-03-26 18:01:54Z

3

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.

answered Mar 26, 2013 at 18:01

Stanley

1,4515 gold badges20 silver badges36 bronze badges

Collectives™ on Stack Overflow

Multithreaded file write via an existing object

2 Answers 2

3 Comments

Comments

Your Answer

Hot Network Questions

Collectives™ on Stack Overflow

2 Answers 2

3 Comments

Comments

Your Answer

Sign up or log in

Post as a guest

Related