Creating/Using FileStream Thread Safe

Question

In my Application when I write text files (logs, traces, etc), I use TFileStream class. There are cases that I write the data in multithreaded environment, those are the steps:

1- Write Cache Data
2- For each 1000 lines I save to File.
3- Clear Data.

This process is repeated during all processing.

Problem Description:

With 16 threads, the system throws the following exception:

Access Violation - file already in use by another application.
I guess this is happening because that the handle used by one thread is not closed yet, when another thread needs to open.

I changed the architecture to the following: (bellow is the NEW implementation)
In the previous way, the TFileStream was created with FileName and Mode parameters, and destroyed closing the handle (I wasn't using TMyFileStream)

TMyFileStream = class(TFileStream)
public
   destructor Destroy; override;
end;

TLog = class(TStringList)
private
  FFileHandle: Integer;
  FirstTime: Boolean;
  FName: String;
protected
  procedure Flush;
  constructor Create;
  destructor Destroy;
end; 


destructor TMyFileStream.Destroy;
begin
  //Do Not Close the Handle, yet!
  FHandle := -1;
  inherited Destroy;
end;

procedure TLog.Flush;
var
  StrBuf: PChar; LogFile: string;
  F: TFileStream;
  InternalHandle: Cardinal;
begin
  if (Text <> '') then
  begin
    LogFile:= GetDir() + FName + '.txt';
    ForceDirectories(ExtractFilePath(LogFile));
    if FFileHandle < 0 then
    begin
      if FirstTime then
        FirstTime := False;

      if FileExists(LogFile) then
        if not SysUtils.DeleteFile(LogFile) then
          RaiseLastOSError;

      InternalHandle := CreateFile(PChar(LogFile), GENERIC_READ or GENERIC_WRITE,         FILE_SHARE_READ, nil, CREATE_NEW, 0,0);
      if InternalHandle = INVALID_HANDLE_VALUE then
        RaiseLastOSError
      else if GetLastError = ERROR_ALREADY_EXISTS then
      begin
        InternalHandle := CreateFile(PChar(LogFile), GENERIC_READ   or GENERIC_WRITE, FILE_SHARE_READ, nil, OPEN_EXISTING, 0,0);
        if InternalHandle = INVALID_HANDLE_VALUE then
          RaiseLastOSError
        else
          FFileHandle := InternalHandle;
      end
      else
        FFileHandle := InternalHandle;
    end;

    F := TMyFileStream.Create(FFileHandle);
    try
      StrBuf := PChar(Text);
      F.Position := F.Size;
      F.Write(StrBuf^, StrLen(StrBuf));
    finally
      F.Free();
    end;

    Clear;
  end;
end;

destructor TLog.Destroy;
begin
  FUserList:= nil;
  Flush;
  if FFileHandle >= 0 then
    CloseHandle(FFileHandle);
  inherited;
end;

constructor TLog.Create;
begin
  inherited;      
  FirstTime := True;      
  FFileHandle := -1;
end;

There is another better way?
Is this implementation correct?
May I improve this?
My guess about the Handle was right?

All theads use the same Log object.

There is no reentrance, i checked! there is something wrong with the TFileStream.

The Access to the Add is synchronized, I mean, I used critical session, and when it reaches 1000 lines, Flush procedure is called.

P.S: I do not want third-party component, i want to create my own.

Answer 1

Well, for a start, there's no point in TMyFileStream. What you are looking for is THandleStream. That class allows you to supply a file handle whose lifetime you control. And if you use THandleStream you'll be able to avoid the rather nasty hacks of your variant. That said, why are you even bothering with a stream? Replace the code that creates and uses the stream with a call to SetFilePointer to seek to the end of the file, and a call to WriteFile to write content.

However, even using that, your proposed solution requires further synchronization. A single windows file handle cannot be used concurrently from multiple threads without synchronisation. You hint in a comment (should be in the question) that you are serializing file writes. If so then you are just fine.

Answer 2

The threaded solution provided by Marko Paunovic quite nice, however while reviewing the code I noticed a small mistake, perhaps just an oversight in the example but I thought I'd mention it just the same in case someone actually tries to use it as-is.

There is a missing call to Flush in TLogger.Destroy, as a result any unflushed (buffered) data is disgarded when the TLogger object is destroyed.

destructor TLogger.Destroy;
begin
  if FStrings.Count > 0 then
     Flush;

  FStrings.Free;
  DeleteCriticalSection(FLock);

  inherited;
end;

Answer 3

How about:

In each thread, add log lines to a TStringList instance until lines.count=1000. Then push the TStringList onto a blocking producer-consumer queue, immediately create a new TStringList and carry on logging to the new list.

Use one Logging thread that dequeues the TStringList instances, writes them to the file and then frees them.

This isolates the log writes from disk/network delays, removes any reliance on dodgy file-locking and will actually work reliably.

Answer 4

I figured MY MISTAKE.

In first place, I want to apologize for posting this stupid question without a proper way to reproduce the exception. In other words, without a SSCCE.

The problem was a control flag that my TLog class used internally.

This flag was created, when we started to evolve our product a parallel architecture.

As we needed to keep the previous form working (at least until everything was in the new architecture). We created some flags to identify if the object was either the new or old version. One of that flags was named CheckMaxSize.

If CheckMaxSize was enabled, at a certain moment, every data inside the instance of this object in each thread, would be thrown to the main instance, which was in the "main" thread (not the GUI one, because it was a background work). Furthermore, when CheckMaxSize is enabled, TLog should never ever call "flush".

Finally, as you can see, in TLog.Destroy there is no check to CheckMaxSize. Therefore, the problem would happen because the name of the file created by this class was always the same, since it was processing the same task, and when One object created the file and another one tried to create another file with the same name, inside the same folder, the OS (Windows) rose an Exception.

Solution:

Rewrite the destructor to:

destructor TLog.Destroy;
begin      
  if CheckMaxSize then
    Flush;
  if FFileHandle >= 0 then
    CloseHandle(FFileHandle);
  inherited;
end;

Answer 5

If you have multithreaded code that needs to write to single file, it's best to have as much control as you can in your hands. And that means, avoid classes which you are not 100% sure how they work.

I suggest that you use multiple threads > single logger architecture, where each thread will have reference to logger object, and add strings to it. Once 1000 lines are reached, logger would flush the collected data in file.

• There is no need to use TFileStream to write data to file, you can go with CreateFile()/SetFilePointer()/WriteFile(), as David already suggested
• TStringList is not thread-safe, so you have to use locks on it

main.dpr:

{$APPTYPE CONSOLE}

uses
  uLogger,
  uWorker;

const
  WORKER_COUNT = 16;

var
  worker: array[0..WORKER_COUNT - 1] of TWorker;
  logger: TLogger;
  C1    : Integer;

begin
  Write('Creating logger...');
  logger := TLogger.Create('test.txt');
  try
    WriteLn(' OK');
    Write('Creating threads...');
    for C1 := Low(worker) to High(worker) do
    begin
      worker[C1] := TWorker.Create(logger);
      worker[C1].Start;
    end;
    WriteLn(' OK');

    Write('Press ENTER to terminate...');
    ReadLn;

    Write('Destroying threads...');
    for C1 := Low(worker) to High(worker) do
    begin
      worker[C1].Terminate;
      worker[C1].WaitFor;
      worker[C1].Free;
    end;
    WriteLn(' OK');
  finally
    Write('Destroying logger...');
    logger.Free;
    WriteLn(' OK');
  end;
end.

uWorker.pas:

unit uWorker;

interface

uses
  System.Classes, uLogger;

type
  TWorker = class(TThread)
  private
    FLogger: TLogger;

  protected
    procedure Execute; override;

  public
    constructor Create(const ALogger: TLogger);
    destructor Destroy; override;
  end;

implementation


function RandomStr: String;
var
  C1: Integer;
begin
  result := '';
  for C1 := 10 to 20 + Random(50) do
    result := result + Chr(Random(91) + 32);
end;


constructor TWorker.Create(const ALogger: TLogger);
begin
  inherited Create(TRUE);

  FLogger := ALogger;
end;

destructor TWorker.Destroy;
begin
  inherited;
end;

procedure TWorker.Execute;
begin
  while not Terminated do
    FLogger.Add(RandomStr);
end;

end.

uLogger.pas:

unit uLogger;

interface

uses
  Winapi.Windows, System.Classes;

type
  TLogger = class
  private
    FStrings        : TStringList;
    FFileName       : String;
    FFlushThreshhold: Integer;
    FLock           : TRTLCriticalSection;

    procedure LockList;
    procedure UnlockList;
    procedure Flush;
  public
    constructor Create(const AFile: String; const AFlushThreshhold: Integer = 1000);
    destructor Destroy; override;

    procedure Add(const AString: String);

    property FlushThreshhold: Integer read FFlushThreshhold write FFlushThreshhold;
  end;

implementation

uses
  System.SysUtils;

constructor TLogger.Create(const AFile: String; const AFlushThreshhold: Integer = 1000);
begin
  FFileName := AFile;
  FFlushThreshhold := AFlushThreshhold;
  FStrings := TStringList.Create;

  InitializeCriticalSection(FLock);
end;

destructor TLogger.Destroy;
begin
  FStrings.Free;
  DeleteCriticalSection(FLock);

  inherited;
end;

procedure TLogger.LockList;
begin
  EnterCriticalSection(FLock);
end;

procedure TLogger.UnlockList;
begin
  LeaveCriticalSection(FLock);
end;

procedure TLogger.Add(const AString: String);
begin
  LockList;
  try
    FStrings.Add(AString);
    if FStrings.Count >= FFlushThreshhold then
      Flush;
  finally
   UnlockList;
  end;
end;

procedure TLogger.Flush;
var
  strbuf  : PChar;
  hFile   : THandle;
  bWritten: DWORD;
begin
  hFile := CreateFile(PChar(FFileName), GENERIC_WRITE, FILE_SHARE_READ, nil, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, 0);
  try
    strbuf := PChar(FStrings.Text);
    SetFilePointer(hFile, 0, nil, FILE_END);
    WriteFile(hFile, strbuf^, StrLen(strbuf), bWritten, nil);
    FStrings.Clear;
  finally
    CloseHandle(hFile);
  end;
end;

end.