When you use TIniFile with CacheUpdates := true, then when you free the TIniFile an error can occur if the component was unable to update the changes (in UpdateFile), this will then raise an exception, which in turn will lead to not calling inherited Destroy, thus leading to memory loss, which cannot be prevented anymore in this case.

Consider the following program:

program test;
{$mode objfpc}{$H+}
uses sysutils, inifiles;
program test;
{$mode objfpc}{$H+}
{$R+}
uses sysutils, inifiles;
VAR
  Ini: TIniFile;
const
  InvalidFilename = '/nonexisting/<|>';
BEGIN
  try//try..finally
    try//try..ecxcept
      Ini := Nil;
      Ini := tIniFile.Create(InvalidFileName);
      Ini.CacheUpdates := True;
      Ini.WriteString('Test','Item1','Value1');
      if ParamCount > 0 then Ini.UpdateFile;
    except
      writeln('Exception handled');
    end;
  finally
    if Assigned(Ini) then
    begin
      try
        Ini.Free;
      except
        writeln('Exception during free');
      end;
    end;
  end;
END.

fpc -gh test.pp

Free Pascal Compiler version 2.4.2 [2010/11/08] for i386 Copyright (c) 1993-2010 by Florian Klaempfl
Target OS: Linux for i386
Compiling test.pp
Linking test
32 lines compiled, 0.9 sec

./test

Exception during free

Heap dump by heaptrc unit
51 memory blocks allocated : 2022/2176
42 memory blocks freed : 1896/2040
9 unfreed memory blocks : 126
True heap size : 425984
True free heap : 425152
Should be : 425272
[..snipped..]

./test 1

Exception handled
Exception during free

Heap dump by heaptrc unit
70 memory blocks allocated : 2494/2728
61 memory blocks freed : 2368/2592
9 unfreed memory blocks : 126
True heap size : 425984
True free heap : 425152
Should be : 425272
[..snipped..]

Should I report this behaviour as a bug, or is it considered "by design"? (My old Delphi does not support the CacheUpdates property, so I cannot test how it behaves in Delphi)

The memory loss (and the raising of the exception) could be prevented lik this:

destructor TIniFile.destroy;
begin
  If FDirty and FCacheUpdates then
  begin
    try
      UpdateFile;
    except
      //just eat the exception
    end;
  end;
  inherited destroy;
end;

Alternatively in UpdateFile the calls to SaveToFile/SaveToSream could be placed inside a try..except block.

Bart

[snip]

Should I report this behaviour as a bug, or is it considered "by design"? (My old Delphi does not support the CacheUpdates property, so I cannot test how it behaves in Delphi)

Please report it as a bug. Your fix proposal is good.

Michael.

Hello FPC-Pascal,

you wrote

> Should I report this behaviour as a bug, or is it considered "by design"? > (My old Delphi does not support the CacheUpdates property, so I cannot > test how it behaves in Delphi)

mvwb> Please report it as a bug. Your fix proposal is good.

I think the fix is not good, eat an exception is not good at all, so I think better:

destructor TIniFile.destroy;
begin
  try
    If FDirty and FCacheUpdates then
    begin
      UpdateFile;
    end;
  finally
    inherited destroy;
  end;
end;

Silently not updating the file seems to be a no-no.

This however means that the programmer has to protect TIniFile.Free with a Try..Except block, which is also rather unusual.

Bart

Hello FPC-Pascal,

Thursday, March 31, 2011, 6:52:26 PM, you wrote:

B> This however means that the programmer has to protect TIniFile.Free B> with a Try..Except block, which is also rather unusual.

It's unusual, but there is an error, or the class does not perform the save in the free procedure and uses a "flush" (forced) or the class must raise an exception as the expected job has not been performed.

procedure Example;
var
  INI: TIniFile;
  Earns: integer;
begin
  Earns:=5000;
  INI:=TIniFile.Create('bank-account');
  INI.CacheUpdates:=true;
  INI.WriteString('Money','earned',Earns);
  INI.Free;
  writeln('Great, this month $',Earns,' are in the pocket!');
end;

The job of the destructor, however, is to clean up the object and return memory. This should never fail IMHO.

One could even argue that the whole call to UpdateFile in the destructor simply should not be there.
If the user (programmer) takes the risk of caching the write updates, then it is the programmers responsability to make sure the cached values are written at some point.

Tipically I would do something like

begin
  try
    Ini := TIni.Create(AFilename);
    try
      Ini.CacheUpdates := True;
      Ini.WriteString(SomeSection, SomeIdent, SomeValue);
      //lots of writing
      Ini.UpdateFile; //flush bugger to file
    except
      ShowMessage('Unable to write to config file');
    end;
  finally
    Ini.Free;
  end;
end;

I would the expect that all (writing) errors are trapped, and the finally statement ensures cleaning up.

I have come to understand that Delphi does it in such a way. If writing fails, the destructor is still (silently) done.

Bart

Hello FPC-Pascal,

Friday, April 1, 2011, 12:17:30 AM, you wrote:

B> The job of the destructor, however, is to clean up the object and return memory. B> This should never fail IMHO.

The clean up has not been successfully performed as in this class a file update is part of the clean up. There are other classes that may fail in the free like ThreadList, TBinaryObjectWriter, and many others that call user code in the destroy. IMHO the free should only raise an exception in serious cases, and the main job of the class is not performed.

B> One could even argue that the whole call to UpdateFile in the B> destructor simply should not be there.

I agree.

B> If the user (programmer) takes the risk of caching the write updates, B> then it is the programmers responsability to make sure the cached B> values are written at some point.

Exactly so it must never update the file in the destroy.

B> Tipically I would do something like
B> begin
B>   try
B>     Ini := TIni.Create(AFilename);
B>     try
B>       Ini.CacheUpdates := True;
B>       Ini.WriteString(SomeSection, SomeIdent, SomeValue);
B>       //lots of writing
B>       Ini.UpdateFile; //flush bugger to file
B>     except
B>       ShowMessage('Unable to write to config file');
B>     end;
B>   finally
B>     Ini.Free;
B>   end;
B> end;
B> I would the expect that all (writing) errors are trapped, and the
B> finally statement ensures cleaning up.

If you write that code you will never get an exception in the free.

B> I have come to understand that Delphi does it in such a way. B> If writing fails, the destructor is still (silently) done.

I do not know how Delphi does it :-?

The problem is Delphi compatibility.

If you use TMemIniFile, then you know that you must call UpdateFile.

When using TIniFile, however, Delphi uses the windows API. This makes sure that each write is automatically committed to Disk. (well, we hope so)

Since this is not portable, the FPC TIniFile in fact descends from TMemIniFile. It should be compatible to Delphi's TiniFile, i.e. the programmer should not worry about 'updating the file', this should be done automatically.

There are 2 options then:

1. Do an UpdateFile after each write.

This used to be the default behaviour, but is VERY slow because each write statement causes the entire file to be written to disk.

2. Cache the updates, and make sure they are written when the instance is destroyed.

This is the current behaviour. It works fast, but the corner case you encountered (an error when updating the file in the destructor) is now an issue.

Going back to 1 is not an option.
For case 2, I am inclined to say that the destructor must always succeed. Errors in destructors are very ugly to trap.

Michael.

In our previous episode, Michael Van Canneyt said:

Going back to 1 is not an option. For case 2, I am inclined to say that the destructor must always succeed. Errors in destructors are very ugly to trap.

... time to mark tinifile as deprecated ?

Hello FPC-Pascal,

Friday, April 1, 2011, 9:58:24 AM, you wrote:

MVC> 2. Cache the updates, and make sure they are written when the instance is destroyed. MVC> This is the current behaviour. It works fast, but the corner case you encountered MVC> (an error when updating the file in the destructor) is now an issue. MVC> Going back to 1 is not an option.
MVC> For case 2, I am inclined to say that the destructor must always succeed. MVC> Errors in destructors are very ugly to trap.

As the destructor must allways success...:

procedure TObject.Free;
begin
  try
    if Assigned(Self) then Self.Destroy;
  except
  end;
end;

If the free would write it must ensures expected job is done, if not an exception must be raised. The other possibility is to never write on free or the job is not deterministic as it could be written or not and you will not be notified about it. In other words, or UpdateFile is mandatory to write or the exception must be raised in the case of error in free.

An exception in free is an extreme rare condition, but it could happend, and is better that your software stops to work than notify 2 years later that your 2 zillions of INI files were not written at all because the user write a non valid file path.

José Mejuto wrote

An exception in free is an extreme rare condition, but it could happend, and is better that your software stops to work than notify 2 years later that your 2 zillions of INI files were not written at all because the user write a non valid file path.

The test case was an invalid filename. Is this the primary failure method? If so, then check this in the create, or the first write, (or check any other reasons why writing to the file would fail) and throw the exception there. Then you should rarely if ever get to the destructor with an unwritable file. Sure, someone could change the permissions on you between the create and the destroy, but how likely is that?

Jeff.

DaWorm wrote

The test case was an invalid filename. Is this the primary failure method?

This was merely to demonstrate the possibility of the destructor failing.

Bart

Bart wrote

> The test case was an invalid filename.  Is this the primary failure > method? This was merely to demonstrate the possibility of the destructor failing.

What other things would make the destructor of the TIniFile fail? It seems to me like a lot of effort to trap something that will rarely happen. You can check for the likely things to go wrong up front (invalid filename, path doesn't exist, no write permissions, disk quota full, whatever else seems likely), and that would eliminate almost anything that could possibly go wrong later (for this particular component), apart from the aforementioned someone messing with the file system behind your back. Sure, it isn't "complete", but it is probably "sufficient".

Jeff.

Hello FPC-Pascal,

Friday, April 1, 2011, 2:45:38 PM, you wrote:

José Mejuto wrote

> An exception in free is an extreme rare condition, but it could > happend, and is better that your software stops to work than notify 2 > years later that your 2 zillions of INI files were not written at all > because the user write a non valid file path.

D> The test case was an invalid filename. Is this the primary failure D> method? If so, then check this in the create, or the first write, (or D> check any other reasons why writing to the file would fail) and throw D> the exception there. Then you should rarely if ever get to the D> destructor with an unwritable file. Sure, someone could change the D> permissions on you between the create and the destroy, but how likely D> is that?

The "problem" is not in the concrete object, the "problem" is if a .free can/shoud raise an exception or not. My idea is that it is not desirable if it can be avoided, but if something does not do whichever it is being designed for, it must raise an exception or the programmer will never notice that something unexpected has happend.

DaWorm wrote

On Fri, Apr 1, 2011 at 9:09 AM, Bart wrote: It seems to me like a lot of effort to trap something that will rarely happen.

But it does happen.

I don't mind that it raises an exception in Destroy (although I find it rather odd), but the consequences of raising the exception _in that place_ are unacceptable IMHO: the object will not be destroyed.

Further more it's not Delphi compatible.

Bart