Is TCSVDataSet fixed?

[egsuh]

In the older versions, there were memory leak when I created and destroyed TCSVDataSet, which does not happen in the recent version. Has the bug been fixed?

[egsuh]

No. I think this is not related with version. I found the memory blocks are not freed when I try to save it to stream.  Following code is to send CSVDataSet to web server.

Code: Pascal  [Select][+][-]

1. function Taq_r_6.UploadSampleList(tpid: string; tsid: integer; filename:string
2.    ): Boolean;
3. var
4.    AStrm: TStream;
5.    AStrl : TStringList;
6.    turl: string;
7.  
8. begin
9.    AStrl := TStringList.Create;
10.    AStrm := TMemoryStream.Create;
11.  
12.    CSVDSet.CSVOptions.FirstLineAsFieldNames:= True;
13.    CSVDSet.LoadFromCSVFile(filename);
14.    CSVDSet.SaveToCSVStream(AStrm);  // <== Memory leak is reported here.
15.  
16.    turl:= URL + Format('SetDataSet?sid=%d', [tsid]);
17.    Result := HttpPostFile(turl,'SampleList', tpid + '.csv', AStrm, AStrl);
18.    AStrm.Position:= 0;
19.    AStrl.Free;
20.    AStrm.Free;
21. end;

But following codes, which use TBufDataSet instead of TCSVDataSet, does not cause memory leak.

Code: Pascal  [Select][+][-]

1. function Taq_r_6.UploadQuestionnaire(tpid: string;DataSet:TDataSet): Boolean;
2. var
3.    turl: string;
4.    AStrm: TStream;
5.    AStrl : TStringList;
6. begin
7.    Result:= false;
8.    DataSet.First;
9.    AStrm := TMemoryStream.Create;
10.    AStrl := TStringList.Create;
11.    try
12.       (DataSet as TBufDataSet).SaveToStream(AStrm);
13.       turl := url + Format('SetDataSet?pid=%s', [tpid]);
14.       Result := HttpPostFile(tURL, 'qdef', tpid + '.bds', AStrm, AStrl);
15.       Result:= uppercase(trim(astrl.Text)) = uppercase(BoolToStr(true, true));
16.    finally
17.      AStrl.Free;
18.      AStrm.Free;
19.    end;
20. end;
21.  

What could be possible reason?

[egsuh]

Well, following gives me peace ^^.

Code: Pascal  [Select][+][-]

1. function Taq_r_6.UploadSampleList(tpid: string; tsid: integer; filename:string
2.    ): Boolean;
3. var
4.    AStrm: TMemoryStream;
5.    AStrl : TStringList;
6.    turl: string;
7.  
8. begin
9.    AStrl := TStringList.Create;
10.    AStrm := TMemoryStream.Create;
11.  
12.    AStrm.LoadFromFile(filename);
13.  
14.    turl:= URL + Format('SetDataSet?sid=%d', [tsid]);
15.    Result := HttpPostFile(turl,'SampleList', tpid + '.csv', AStrm, AStrl);
16.                           // (URL, FieldName, FileName, Data, ResultData)
17.    AStrm.Position:= 0;
18.    AStrl.Free;
19.    AStrm.Free;
20. end;
21.  

[PascalDragon]

No. I think this is not related with version. I found the memory blocks are not freed when I try to save it to stream.

Would you please provide a simple, self contained example that shows this leak and report it?

[Thaddy]

The free's are called in the wrong order:

Code: Pascal  [Select][+][-]

1. // mind the creation order:
2.    AStrl := TStringList.Create;
3.    AStrm := TMemoryStream.Create;
4.  
5. // and here you do the free in the wrong order
6.    AStrl.Free;
7.    AStrm.Free;
8. // which should be in the correct order
9.    AStrm.Free;
10.    AStrl.Free;

First create should be last free'd. IOW LIFO modeled, not FIFO
Since the stream has a dependency on the list that may cause your problem.

It may very well be that there is another bug elsewhere, but you should adhere to Last created should be first to free.
Your code contains a programmer error in the above code.

(By the way,) I would use two try finally's in the above code:

Code: Pascal  [Select][+][-]

1.  
2.   AStrl := TStringList.Create;
3.   try
4.     AStrm := TMemoryStream.Create;
5.     try
6. .....
7.     finally
8.       Astrm.Free;
9.     end;
10.   finally
11.     Astrl.free;
12.   end;

[egsuh]

Would you please provide a simple, self contained example that shows this leak and report it?

Please unzip the attached project, and run it. Following codes are all.

Code: Pascal  [Select][+][-]

1. procedure TForm1.Button1Click(Sender: TObject);
2. var
3.    AStream: TStream;
4. begin
5.    od1.filter := 'CSV files|*.csv';
6.    if od1.execute then begin
7.       CSVDataSet1.CSVOptions.FirstLineAsFieldNames:= True;
8.       CSVDataSet1.LoadFromCSVFile(od1.filename);
9.       CSVDataSEt1.Open;
10.  
11.       AStream:= TMemoryStream.Create;
12.       CSVDataSet1.SaveToCSVStream(AStream);
13.       // CSVDataSet1.SaveToStream(AStream); // this works fine, but following file is not readable.
14.  
15.       AStream.Position := 0;
16.       (AStream as TMemoryStream).SaveToFile(ChangeFileExt(od1.filename, '_2.csv'));
17.       AStream.Free;
18.       ShowMessage('Done');
19.    end;
20. end;
21.  

[wp]

Here is a simpler version of the test project. It confirms that there is a memory leak. It seems to be in the line with TCSVDataset.SaveToCSVFile - when this is removed the memory leak is gone.

Please file a bug report in the FPC bug tracker and add my test program (along with a simple data file).

Code: Pascal  [Select][+][-]

1. program csvdataset_memleak;
2.  
3. uses
4.   db, csvdataset;
5.  
6. var
7.   dataset: TCSVDataset;
8. begin
9.   dataset := TCSVDataset.Create(nil);
10.   try
11.     dataset.CSVOptions.FirstLineAsFieldNames:= True;
12.     dataset.LoadFromCSVFile('test.csv');
13.     dataset.SaveToCSVFile('test-2.csv');
14.     WriteLn('done');
15.   finally
16.     dataset.Free;
17.   end;
18. end.

[Thaddy]

It seems that the underlying auto-created stream (for SaveToFile) in TCSVDataPacketReader is not free'd.

[wp]

It seems that the underlying auto-created stream (for SaveToFile) in TCSVDataPacketReader is not free'd.

No. It is the internal TCSVBuilder created by TCSVDataPacketReader.StoreFieldDefs. Adding it to TCSVDatapackageReader.Destroy fixes the memory leak:

Code: Pascal  [Select][+][-]

1. destructor TCSVDataPacketReader.Destroy;
2. begin
3.   FreeAndNil(FBuilder);
4.   FreeAndNil(FCreateFieldDefs);
5.   If FOwnsOptions then
6.     FreeAndNil(FOPtions);
7.   FreeAndNil(Fline);
8.   FreeAndNil(FParser);
9.   inherited Destroy;
10. end;

I'll submit this as a patch.

[EDIT]
Done: https://gitlab.com/freepascal.org/fpc/source/-/issues/39607

[PascalDragon]

Done: https://gitlab.com/freepascal.org/fpc/source/-/issues/39607

Thank you.

[wp]

Michael already fixed it.

[PascalDragon]

Michael already fixed it.

Even better.

[egsuh]

Is there anything more than adding following line to csvdataset.pp file to fix this?

FreeAndNil(FBuilder);

I opened csvdataset.pp file, added the above line (and commented out sqldb from uses clause), and built file and then compiled and ran my previous application but the problem still exists.

[dsiders]

Is there anything more than adding following line to csvdataset.pp file to fix this?

FreeAndNil(FBuilder);

I opened csvdataset.pp file, added the above line (and commented out sqldb from uses clause), and built file and then compiled and ran my previous application but the problem still exists.

There were 2 commits:

https://gitlab.com/freepascal.org/fpc/source/-/commit/21326cfb9ab8b66889c9620ea99a14f435b58d5d
https://gitlab.com/freepascal.org/fpc/source/-/commit/56acf11ec227af677531fc5950a7a28420746467

[PascalDragon]

I opened csvdataset.pp file, added the above line (and commented out sqldb from uses clause), and built file and then compiled and ran my previous application but the problem still exists.

How did you build the file? Did you copy it to your own project or did you make sure that the PPU that is provided either with Lazarus (on Windows) or with the FPC installer was updated?