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.

There is no reentrance, i checked! there is something wrong with the TFileStream — EProgrammerNotFound, May 14 '13 at 12:58
The Access to the Add is synchronized, i mean, i used critical session, and when it reaches 1000 lines, Flush procedure is called. — EProgrammerNotFound, May 14 '13 at 12:59
Don't use threads to write data to file. Use them only to process the data, and have reference to unique logger object in each thread, and each thread would add lines to logger object, then call logger.Flush(). Logger will effectively act as bottleneck of the system, which is required in this case. — rsrx, May 14 '13 at 13:32
All threads have their own log and at a certain time, the threads send the text to a main thread, which is responsible for flushing, if necessary, ie if reached 1000 lines. — EProgrammerNotFound, May 14 '13 at 13:36
TStringList isn't thread safe. You will get AV error if you try to call .Add() from multiple threads to single TStringList object. — rsrx, May 14 '13 at 13:54
@MatheusFreitas You don't say that in the question. You only said it in comments. You should put important details in the question rather than comments. People won't read all the comments. — David Heffernan, May 14 '13 at 14:07
Yes, I did, and you have some flaw in mechanism which is causing AV error. I will post an answer now including full source that works. — rsrx, May 14 '13 at 14:08

David Heffernan · Accepted Answer · 2013-05-14T13:16:30.310

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.

edited May 14 '13 at 13:16

answered May 14 '13 at 13:08

David Heffernan

601,492
42
1,072
1,490

I used TFileStream because the buffering, i didn't knew about WriteFile, may i use GetFileSize to write in the EOF?? – EProgrammerNotFound May 14 '13 at 13:13
`TFileStream` does not use any buffering. Call `SetFilePointer` to seek to the end of the file. – David Heffernan May 14 '13 at 13:15
Does WriteFile do buffering? – EProgrammerNotFound May 14 '13 at 13:19
Well, just the standard Windows OS file system buffering. But nothing beyond that. What made you think that `TFileStream` had buffering? It's just a loose wrapper around `WriteFile`. – David Heffernan May 14 '13 at 13:22
I read a post on this site, stackoverflow...now I learned;"only trust in the books!!" – EProgrammerNotFound May 14 '13 at 13:25
i'll study about buffering... sorry for saying without know for sure. – EProgrammerNotFound May 14 '13 at 13:26
Is my code right? SetFilePointer(Handle, getfilesize(logfile), Apointer, FILE_BEGIN) – EProgrammerNotFound May 14 '13 at 13:51
That's the hard way. Use `FILE_END` instead. – David Heffernan May 14 '13 at 13:54
`SetFilePointer(Handle, 0, nil, FILE_END)` is what you need – David Heffernan May 14 '13 at 13:57

score 1 · Answer 2 · answered Sep 01 '13 at 15:52

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;

score 0 · Answer 3 · answered May 14 '13 at 13:13

0

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.

answered May 14 '13 at 13:13

Martin James

24,453
3
36
60

I can't do this, will consume a lot of memory – EProgrammerNotFound May 14 '13 at 13:14
I used filestream because with it, i don't need to load again the file. – EProgrammerNotFound May 14 '13 at 13:15
A lot of memory? You're cacheing/buffering 1000 lines per thread anyway? – Martin James May 14 '13 at 13:17
'i don't need to load again the file' - don't close it. – Martin James May 14 '13 at 13:17
double the memory consumption is meaningless – EProgrammerNotFound May 14 '13 at 13:19
..or better, flush/close it in the logger thread if its queue-wait times out, eg. if no log entries received for, say, 500ms. – Martin James May 14 '13 at 13:19
Your idea about parallelization is quite good, but i can't duplicate the strings.. – EProgrammerNotFound May 14 '13 at 13:23
I'm not cacheing / buffering 1000 lines per thread. There is a single log object and all threads use it. Doesn't matter if the thread add 1000 lines or 1, when it reaches 1000 will be flushed. – EProgrammerNotFound May 14 '13 at 13:33

EProgrammerNotFound · Answer 4 · 2014-03-11T16:11:49.003

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;

rsrx · Answer 5 · 2013-05-14T14:27:51.927

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.

If you're getting AV error, then no, it isn't. Or problem lays somewhere else in your code. — rsrx, May 14 '13 at 17:35
Code works fine. You obviously haven't even tested it and just talk without any proofs. Anyway, as you said, question has been answered and I won't bother anymore. — rsrx, May 14 '13 at 17:48

Creating/Using FileStream Thread Safe

5 Answers5