Recent

Author Topic: Is TCSVDataSet fixed?  (Read 2360 times)

egsuh

  • Hero Member
  • *****
  • Posts: 943
Is TCSVDataSet fixed?
« on: March 01, 2022, 01:54:28 am »
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

  • Hero Member
  • *****
  • Posts: 943
Re: Is TCSVDataSet fixed?
« Reply #1 on: March 01, 2022, 04:47:48 am »
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

  • Hero Member
  • *****
  • Posts: 943
Re: Is TCSVDataSet fixed?
« Reply #2 on: March 01, 2022, 04:56:41 am »
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

  • Hero Member
  • *****
  • Posts: 4301
  • Compiler Developer
Re: Is TCSVDataSet fixed?
« Reply #3 on: March 01, 2022, 09:02:15 am »
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

  • Hero Member
  • *****
  • Posts: 11773
Re: Is TCSVDataSet fixed?
« Reply #4 on: March 01, 2022, 09:36:49 am »
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;
« Last Edit: March 01, 2022, 10:09:00 am by Thaddy »
Black themes should be banned.

egsuh

  • Hero Member
  • *****
  • Posts: 943
Re: Is TCSVDataSet fixed?
« Reply #5 on: March 01, 2022, 10:46:29 am »
Quote
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

  • Hero Member
  • *****
  • Posts: 9908
Re: Is TCSVDataSet fixed?
« Reply #6 on: March 01, 2022, 11:30:45 am »
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

  • Hero Member
  • *****
  • Posts: 11773
Re: Is TCSVDataSet fixed?
« Reply #7 on: March 01, 2022, 12:19:26 pm »
It seems that the underlying auto-created stream (for SaveToFile) in TCSVDataPacketReader is not free'd.
Black themes should be banned.

wp

  • Hero Member
  • *****
  • Posts: 9908
Re: Is TCSVDataSet fixed?
« Reply #8 on: March 01, 2022, 12:42:01 pm »
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
« Last Edit: March 01, 2022, 01:46:17 pm by wp »

PascalDragon

  • Hero Member
  • *****
  • Posts: 4301
  • Compiler Developer

wp

  • Hero Member
  • *****
  • Posts: 9908
Re: Is TCSVDataSet fixed?
« Reply #10 on: March 01, 2022, 01:40:28 pm »
Michael already fixed it.

PascalDragon

  • Hero Member
  • *****
  • Posts: 4301
  • Compiler Developer
Re: Is TCSVDataSet fixed?
« Reply #11 on: March 01, 2022, 01:41:37 pm »
Michael already fixed it.

Even better. :)

egsuh

  • Hero Member
  • *****
  • Posts: 943
Re: Is TCSVDataSet fixed?
« Reply #12 on: March 02, 2022, 03:52:38 am »
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

  • Hero Member
  • *****
  • Posts: 673
Re: Is TCSVDataSet fixed?
« Reply #13 on: March 02, 2022, 04:20:01 am »
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

Preview Lazarus 2.3.0 documentation at: https://dsiders.gitlab.io/lazdocsnext

PascalDragon

  • Hero Member
  • *****
  • Posts: 4301
  • Compiler Developer
Re: Is TCSVDataSet fixed?
« Reply #14 on: March 02, 2022, 09:11:29 am »
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?

 

TinyPortal © 2005-2018