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.