Thread leaks report (FastMM4)

[alogrep 0]

Hi everybody.

I hope somebody could help me with this "leaks" report from FastMM4..

I have no idea what BaseThreadInitThunk and similar mean..

 

--------------------------------2024/6/27 17:52:48--------------------------------
A memory block has been leaked. The size is: 20
This block was allocated by thread 0x38E4, and the stack trace (return addresses) at the time was:
C775C2 [System.pas][System][@GetMem$qqri][4962]
C7C6F8 [System.pas][System][@StartExe$qqrp23System.PackageInfoTablep17System.TLibModule][24306]
75F2FCC9 [BaseThreadInitThunk]
77AD7CBE [RtlGetAppContainerNamedObjectPath]
77AD7C8E [RtlGetAppContainerNamedObjectPath]
The block is currently used for an object of class: IdThreadSafe.TIdThreadSafeInteger
The allocation number is: 7395
--------------------------------2024/6/27 17:52:48--------------------------------
A memory block has been leaked. The size is: 36
This block was allocated by thread 0x38E4, and the stack trace (return addresses) at the time was:
C775C2 [System.pas][System][@GetMem$qqri][4962]
18F665B
C7C6F8 [System.pas][System][@StartExe$qqrp23System.PackageInfoTablep17System.TLibModule][24306]
75F2FCC9 [BaseThreadInitThunk]
77AD7CBE [RtlGetAppContainerNamedObjectPath]
77AD7C8E [RtlGetAppContainerNamedObjectPath]
The block is currently used for an object of class: IdGlobal.TIdCriticalSection
The allocation number is: 6886.

This is my coce;

 

TTCPEchoDaemon = class(TThread)
  private
    Sock:TTCPBlockSocket;
  public
    Constructor Create;
    Destructor Destroy; override;
    procedure Execute; override;
  end;


Constructor TTCPEchoDaemon.Create;
begin
  inherited create(true);
  if self.Handle = 0 then
      raise EThread.CreateResFmt(@SThreadCreateError, ['ECHO Daemon '+SysErrorMessage(GetLastError)]);
  FreeOnTerminate:=true;
  resume;
  sock:=TTCPBlockSocket.create;
end;


procedure TTCPEchoDaemon.Execute;
var
  ClientSock:TSocket;
begin
  with sock do begin
      CreateSocket;
      setLinger(true,10000);
      bind(SERVERIP,portnumber);
      listen;
      repeat
        if terminated or _stopdeamon then break;
        if canread(1000) then   begin
    // *** there is code here, but for this test it
    //  *** never gets execute b/c nothing is sent. 
        end;
      until false;
    end;
end;

Destructor TTCPEchoDaemon.Destroy;
begin
  Sock.free;
  inherited destroy;
end;

 

[Tommi Prami 130]

At least exception raising in constructor might not be best practise.

-Tee-

[Kas Ob. 121]

Hi,

6 hours ago, alogrep said:

I have no idea what BaseThreadInitThunk and similar mean..

Don't concern yourself with this one, it is irrelevant to your code, you should for/at the calls after it.

6 hours ago, alogrep said:

I hope somebody could help me with this "leaks" report from FastMM4..

Both allocation are reported from this line

Quote

C7C6F8 [System.pas][System][@StartExe$qqrp23System.PackageInfoTablep17System.TLibModule][24306]

This where you registered packages being loaded, this means it might be one package (or two) had been loaded and not unloaded in orderly manner, or/and this package had allocated memory in its initialization and never freed it.

Quote

C775C2 [System.pas][System][@GetMem$qqri][4962]
18F665B  <----   this address belongs to the offending package (or the not freed class/variable/record)
C7C6F8 [System.pas][System][@StartExe$qqrp23System.PackageInfoTablep17System.TLibModule][24306]

 

Find the runtime package you are using and fix it.

 

Also as @Tommi Prami pointed, try to not raise exception in constructors and destructor.

Your TTCPEchoDaemon code you pasted is irrelevant to the memory leak, but if you are doubting it then i inclined to remind you, to check how and where are declared and initialize this EchoDaemon, it could be the leak as the size of the leak is conveniently close enough to the size of your class. 

 

In other and short words, the leak is happening in initialization section in one unit (most likely one unit).

[Der schöne Günther 316]

I am astounded at both your claim that throwing exceptions in the constructor should generally be avoided.

[havrlisan 25]

Just now, Der schöne Günther said:

I am astounded at both your claim that throwing exceptions in the constructor should generally be avoided.

Raising exceptions in constructor will lead to memory leaks, unless the exception is raised before the inherited call. I don't see a problem with their claims.

[Kas Ob. 121]

9 minutes ago, Der schöne Günther said:

I am astounded at both your claim that throwing exceptions in the constructor should generally be avoided.

Because there is a very popular of miss practice with Pascal and Delphi which is (ab)using the constructor and destructor for everything, i do it, and every one is doing it, even the RTL since Borland is did it ..

 

and to be clear the ones that should be used for any sort memory initialization, object creation, or what ever other than initialize values for the unmanaged object fields are

    procedure AfterConstruction; virtual;
    procedure BeforeDestruction; virtual;

These should be used for creating object fields/variables and/or initializing values for managed fields...etc , with these you are free to raise what ever you want, a nice exception will be raised and a nice memory leaks will be reported, in other words the fail will be graceful and easy to locate.

 

Another approach:

Also as in OP class above, if you need to raise an exception then in my opinion the class design can be written better by separate Create and adding an Init (or Start...), Create to just create and another for functionality that might fail, this will %100 be memory leak proof no matter what is the case, and if you think about it, create doesn't raise and will not fail, while the failure might come form something like an Init procedure, here failed or exception raised or what ever happen, Create didn't (and will not) break the logic flow of the application and Free is guaranteed to work.

 

In case of wondering what will happen with Out of Memory, then either Create will fail from the RTL, or will not fail because we don't allocate or create anything in the constructor, and that is the point of this suggestion.

[Der schöne Günther 316]

I am absolutely bewildered.

24 minutes ago, havrlisan said:

Raising exceptions in constructor will lead to memory leaks, unless the exception is raised before the inherited call.

That is absolutely not true.

 

See:

Methods (Delphi) - RAD Studio (embarcadero.com)

Quote

When an exception is raised during the creation of an object, Destroy is automatically called to dispose of the unfinished object.

 

or:

https://stackoverflow.com/a/39110161

 

 

4 minutes ago, Kas Ob. said:

and to be clear the ones that should be used for any sort memory initialization, object creation, or what ever other than initialize values for the unmanaged object fields are

    procedure AfterConstruction; virtual;
    procedure BeforeDestruction; virtual;

This is the first time I ever heard something like this.

[dummzeuch 1506]

I see no problem with raising an exception in a constructor, provided you write the destructor in a way that can handle a partly constructed instance. Since you can't prevent any system or RTL exception to be raised from within the constructor, you'll have to handle that case anyway.

Always keep in mind that an exception in a constructor will cause the destructor being called immediately.

 

So don't do this:

constructor TBla.Create;
begin
  inherited;
  FSomeHelperObject := TSomeClass.Create;
end;

destructor TBla.Destroy;
begin
  FSomeHelperObject.Free; // <== this might cause an AV if FSomeHelperObject hasn't been assigend
  inherited;
end;

But do this instead:

destructor TBla.Destroy;
begin
  if Assigned(FSomeHelperObject) then
    FSomeHelperObject.Free;
  inherited;
end;

(Or use FreeAndNil, which basically does the same internally.)

 

You can easily test if your destructor can handle this case by temporarily putting a raise exception.Create('test') as the first statement in your constructor (before even calling inherited Create).

 

I'm sure we are all guilty of writing wrong destructors at some time though.

Edited June 28 by dummzeuch

[havrlisan 25]

17 minutes ago, Der schöne Günther said:

That is absolutely not true.

I stand corrected. What dummzeuch Dalija wrote below is the correct approach. The reason why I raise exceptions before the inherited call is to make sure nothing gets unnecessarily initialized in parent classes. Destructors should handle both scenarios though.

Edited June 28 by havrlisan
Dalija is right

[Dalija Prasnikar 1399]

7 minutes ago, dummzeuch said:

 

constructor TBla.Create;
begin
  inherited;
  FSomeHelperObject := TSomeClass.Create;
end;

destructor TBla.Destroy;
begin
  FSomeHelperObject.Free; // <== this might cause an AV if FSomeHelperObject hasn't been assigend
  inherited;
end;

 

The above code is fine. Free can be called on nil object reference. Testing whether it is assigned before calling Free is redundant.

[dummzeuch 1506]

Ouch, yes you are right! I chose the wrong example. But there are always more complex examples where this won't work. (Unfortunately I can't think of one right now. GetMem/FreeMem should also work fine.)

 

 

 

[Dalija Prasnikar 1399]

7 hours ago, alogrep said:

 

Constructor TTCPEchoDaemon.Create;
begin
  inherited create(False);  
  FreeOnTerminate := true;
  sock:=TTCPBlockSocket.create;
end;

Destructor TTCPEchoDaemon.Destroy;
begin
  inherited destroy;
  Sock.free; 
end;

 

There are some issues in your code.

 

Your constructor can be simplified - the inherited thread constructor will handle raising exception if thread cannot be created. Also it does not make sense to construct suspended thread and then starting it in constructor because constructor chain will complete before thread runs because non suspended thread is automatically started in AfterConstruction method, not before.

 

Additionally, your code constructs sock after you have called Resume, where it would be possible for thread to access instance which is not created yet (this is very slight possibility, but it is still possible).

 

You should also destroy all objects in destructor, after you call inherited destroy, which will guarantee that thread is no longer running at that point and preventing access to already destroyed sock object.

 

Next, since you are creating self destroying thread, such threads don't have proper cleanup during application shutdown and will just be killed by OS if they are still running at that time. If that happens there will be memory leaks. Explicitly releasing the thread would be better for controlled shutdown and thread cleanup. 

[Kas Ob. 121]

36 minutes ago, Der schöne Günther said:

That is absolutely not true.

 

See:

Methods (Delphi) - RAD Studio (embarcadero.com)

Quote

When an exception is raised during the creation of an object, Destroy is automatically called to dispose of the unfinished object.

 

or:

https://stackoverflow.com/a/39110161

You are missing the point of best practice to ensure memory integrity, 

Yes if an exception is raised in the constructor then the class itself is nil and will not leak memory, as pointed by @Dalija Prasnikar but if it is already allocated/created stuff then this stuff will leak so instead of depending on extra work to ensure non of the "stuff" had leaked just use best practice.

 

On other hand, what pointed by @dummzeuch and @Dalija Prasnikar it does ignore the fact that that FSomeHelperObject is not managed variable hence it might have any value initially (random from the memory), rendering the following worst case scenario by raising unexplained and hard to track exceptions

  if Assigned(FSomeHelperObject) then
    FSomeHelperObject.Free;

In other words, when an exception is raised in the constructor, you can't control or know where the stopping point was, unless considered this at designing, thus if you want to raise exception in the constructor then you need to do the extra work, like assigning FSomeHelperObject to nil before creating it ! and live with the compiler complaining about useless line and most likely the compiler will remove it, again rendering the code vulnerable to unexplained exception.

[Dalija Prasnikar 1399]

Just now, dummzeuch said:

But there are always more complex examples where this won't work.

There are none. Free can always be called on nil instance. Running other code in the destructor where it is assumed that instance is not nil would require checking whether instance is nil before calling that code. In such code Free is often put within the Assigned block, not because it needs to be there but to avoid additional potentially unnecessary call when the instance is nil.

[Dalija Prasnikar 1399]

8 minutes ago, Kas Ob. said:

Yes if an exception is raised in the constructor then the class itself is nil and will not leak memory, as pointed by @Dalija Prasnikar but if it is already allocated/created stuff then this stuff will leak so instead of depending on extra work to ensure non of the "stuff" had leaked just use best practice.

Sorry, but @Der schöne Günther is correct. If the exception is raised in the constructor, then destructor will be called to perform cleanup on already allocated stuff. Unless the destructor is broken (badly written), there will be no memory leaks. That is why destructor needs to be able to work properly on partially constructed object instances, without raising any additional exceptions. Exceptions raised within the destructor will cause irreparable memory leaks.

 

Best practice is that you can do whatever you want in the constructor in any order you whish (provided that you don't access things that are not yet created) and destructor must never raise an exception. Again you can also call inherited destructor in any order if you need to do that for some reason.

Edited June 28 by Dalija Prasnikar

[Kas Ob. 121]

14 minutes ago, Dalija Prasnikar said:

Sorry, but @Der schöne Günther is correct. If the exception is raised in the constructor, then destructor will be called to perform cleanup on already allocated stuff. Unless the destructor is broken (badly written), there will be no memory leaks. That is why destructor needs to be able to work properly on partially constructed object instances, without raising any additional exceptions. Exceptions raised within the destructor will cause irreparable memory leaks.

I am sorry too, but look at this code as example

type
  TFileCopyClass = class
  private
    fFileSrc: TFileStream;
    fFileDst: TFileStream;
  public
    constructor Create(const Source, Destination: string);
    destructor Destroy; override;

  end;

{ TFileCopyClass }

constructor TFileCopyClass.Create(const Source, Destination: string);
begin
  fFileSrc := TFileStream.Create(Source, fmOpenRead);
  fFileDst := TFileStream.Create(Source, fmOpenWrite);
end;

destructor TFileCopyClass.Destroy;
begin
  fFileDst.Free;
  fFileSrc.Free;
  inherited;
end;

What are the value at the "begin" in that constructor ?

The answer is arbitrary and random even if you see it as nil most the time, these are unmanaged fields/variables.

So if TFileStream raised an exception then the value of lets say fFileSrc is undefined and might be not nil, hence causing another unknown exception in the TFileCopyClass.Destroy;

 

Am i right there or i am missing something ?

[msohn 28]

13 minutes ago, Kas Ob. said:

Am i right there or i am missing something ?

You're missing TObject.InitInstance which ensures all fields are initialised before the constructor is called.

 

Edit: link for convenience https://docwiki.embarcadero.com/Libraries/Athens/en/System.TObject.InitInstance

Edited June 28 by msohn
added link

[Tommi Prami 130]

1 hour ago, Der schöne Günther said:

I am absolutely bewildered.

That is absolutely not true.

 

Tested this and I stand corrected.

I bet this was the case way back at least, that exception in constructor was leading to memory leaks. Might be still wrong tpugh.

 

-Tee-

[Kas Ob. 121]

9 minutes ago, msohn said:

You're missing TObject.InitInstance which ensures all fields are initialised before the constructor is called.

Thank you !

[dummzeuch 1506]

41 minutes ago, Dalija Prasnikar said:

44 minutes ago, dummzeuch said:

But there are always more complex examples where this won't work.

There are none. Free can always be called on nil instance.

I wasn't talking bout calling free for an object, but what you were describing next:

44 minutes ago, Dalija Prasnikar said:

Running other code in the destructor where it is assumed that instance is not nil would require checking whether instance is nil before calling that code. In such code Free is often put within the Assigned block, not because it needs to be there but to avoid additional potentially unnecessary call when the instance is nil. 

As I said: Wrong example on my part. I apologize.

[Dalija Prasnikar 1399]

32 minutes ago, dummzeuch said:

I wasn't talking bout calling free for an object, but what you were describing next:

Sorry, I misunderstood then.

[Dalija Prasnikar 1399]

43 minutes ago, Tommi Prami said:

I bet this was the case way back at least, that exception in constructor was leading to memory leaks. Might be still wrong tpugh.

Raising exceptions in constructor never ever lead to memory leaks. Only if the code in destructor is bad and is not able to clean up partially initialized object instances. such situations may look like raising exception in constructor is at fault, while actually it is the destructor that needs to be fixed in such case.

[Tommi Prami 130]

7 minutes ago, Dalija Prasnikar said:

Raising exceptions in constructor never ever lead to memory leaks. Only if the code in destructor is bad and is not able to clean up partially initialized object instances. such situations may look like raising exception in constructor is at fault, while actually it is the destructor that needs to be fixed in such case.

Would be nice to know where this misconception of mine comes from, as it seems that I am not alone. 

I have no recollection where I got that from.,

-Tee-

[Der schöne Günther 316]

9 minutes ago, Tommi Prami said:

Would be nice to know where this misconception of mine comes from, as it seems that I am not alone. 

Most probably because it is like that in many other languages.

[Leif Uneus 43]

Barry Kelly explains it rather good in a comment:

https://herbsutter.com/2008/07/25/constructor-exceptions-in-c-c-and-java/