One more memory leak and FastMM4

[aehimself 396]

21 hours ago, Remy Lebeau said:

Which means the TBetFairApi object that is holding that UnicodeString is not freeing that UnicodeString properly.

Without manually modifying the pointer as you suggested and properly released owner... how exactly you can cause a memory leak with Strings?

I always thought records, arrays behave like interfaces and are automatically discarded when their lifetime is over.

[Guest]

38 minutes ago, aehimself said:

Without manually modifying the pointer as you suggested and properly released owner... how exactly you can cause a memory leak with Strings?

Multi-Threading, there is slight chance he might actually doing this, by checking on FToken in main thread or just different thread, i forgot to ask OP about threading !

One wrong call to OpenSSL with wrong parameter size and it will write over your stack, in fact case it might write anywhere, because Ftoken here is not on the stack but it is somewhere and may be near that SSL handler, if broken API declaration that expecting buffer to fill on return, then it can be still working fine and yet corrupting the memory.

 

44 minutes ago, aehimself said:

I always thought records, arrays behave like interfaces and are automatically discarded when their lifetime is over.

Yes they are all and yet they all have that reference count integer before the pointer ( with negative index from the pointer itself), overflow it and have fun finding the culprit, specially of the overflow was only changing the counter, you will see the data intact and move to look somewhere else, nasty bugs.

[David Heffernan 2345]

All this speculation..... A minimal example and the answer would be simple. 

[aehimself 396]

1 minute ago, Kas Ob. said:

Multi-Threading

Damn, forgot about multithreading, too. +1 for that sharp eye!

 

14 minutes ago, Kas Ob. said:

overflow it and have fun finding the culprit

I never actually thought about this. Running this on a Unicode Delphi shows no memory leaks (with ReportMemoryLeaksOnShutdown := True):

Var
 pc: PChar;
 s: String;
begin
 s := 'Hello, world!';
 GetMem(pc, s.Length + 1);
 Try
  StrPCopy(pc, s);
 Finally
  FreeMem(pc);
 End;
end;

...but make it StrPCopy(pc, s + s); and you have 2 unknown objects leaked. Make it 5 times s and you have 3 unknown objects leaked. I could not find 4 unknown objects without an actual access violation.

I guess it's only the limitations of the shipped FastMM...? Since our company bought it, I'll run this code with DeLeaker tomorrow out of curiosity to see what it will say about it.

 

At the end of the day, however, we all can agree that memory corruption is a bit more serious issue, and - personally - I would not even consider it as a memory leak 🙂

 

Still, an interesting aspect!

[Guest]

16 minutes ago, aehimself said:

Still, an interesting aspect!

Have a look at this if you are looking for interesting stuff, and missed code with corrupt memory/stack

 

[aehimself 396]

We are getting close to the next release so time is not that much to spare, but I managed to run my test. DeLeaker shows about the same results with 1, 2 and 5 copies of "s".

I am no expert with memory leak detection in Delphi, but can someone explain this to me?

 

On 6/25/2020 at 10:18 PM, aehimself said:

 

Var
 pc: PChar;
 s: String;
begin
 s := 'Hello, world!';
 GetMem(pc, s.Length + 1);
 Try
  StrPCopy(pc, s);
 Finally
  FreeMem(pc);
 End;
end;

 

No leaks, however (on a Unicode Delphi) this used up twice as much space as it was reserved. Fine, it's not a leak (as it is properly released), it's corruption. But why StrPCopy(pc, s + s); already pops up as leak in this case?

[Remy Lebeau 1396]

As you noted, that code is not allocating memory correctly.  GetMem() operates on bytes, but s.Length is expressed in characters, not bytes, and SizeOf(Char) is 2 bytes in D2009+, so you need to double the allocation, otherwise you will have a buffer overflow on StrPCopy(), corrupting surrounding memory, which can lead to all kinds of problems, including leaks of other things that were allocated dynamically, if you corrupt the pointers to them:

var
  pc: PChar;
  s: String;
begin
  s := 'Hello, world!';
  GetMem(pc, (s.Length + 1) * SizeOf(Char));
  try
    StrPCopy(pc, s);
  finally
   FreeMem(pc);
  end;
end;

Alternatively, use StrAlloc() instead:

var
  pc: PChar;
  s: String;
begin
  s := 'Hello, world!';
  pc := StrAlloc(s.Length + 1);
  try
    StrPCopy(pc, s);
  finally
   StrDispose(pc);
 end;
end;

In either case, I would not use GetMem() or StrAlloc() at all, I would use a dynamic array instead:

Var
  pc: array of Char; // or TArray<Char>
  s: String;
begin
  s := 'Hello, world!';
  SetLength(pc, s.Length + 1);
  StrPCopy(PChar(pc), s);
end;

Now, with that said, you claim you are seeing "leaks" when you use StrPCopy(pc, s + s) and StrPCopy(pc, s + s + s + s +s).  Well, those are both buffer overflows, since you are allocating only enough memory for 1 string.  It is likely that one of the pointers you are corrupting may be the one used by 'pc' or 's' itself!  You will have to use the debugger to verify that.  Check the values of the pointers before and after calling StrPCopy(), and see if they changed.  Or even put Data Breakpoints on the pointers and see if the breakpoints are getting triggered.

Edited July 2, 2020 by Remy Lebeau

[aehimself 396]

10 hours ago, Remy Lebeau said:

GetMem() operates on bytes, but s.Length is expressed in characters, not bytes, and SizeOf(Char) is 2 bytes in D2009+, so you need to double the allocation, otherwise you will have a buffer overflow on StrPCopy(), corrupting surrounding memory, which can lead to all kinds of problems, including leaks of other things that were allocated dynamically, if you corrupt the pointers to them:

That was exactly my purpose 🙂 Don't worry, this is only for testing and satisfying my curiosity.

10 hours ago, Remy Lebeau said:

It is likely that one of the pointers you are corrupting may be the one used by 'pc' or 's' itself!

I guess this will be the cause of the "leak". Today, I attempted to check the addresses of pc and s, and I can not seem to trigger a memory leak, nor access violation even with StrPCopy(pc, s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s + s);

maybe today I have more free continuous unused memory than the last time...

It makes sense, though. If by accident I managed to corrupt the pointer itself, the release will not happen at the right place (or maybe not happen at all?). I just cannot seem to find what the 3rd leaked allocation might be.

 

Anyway, memory corruption is no job for an allocation detection tool. I just found it strange that sometimes even these were triggered.

[Alberto Paganini 3]

On 6/25/2020 at 8:54 PM, David Heffernan said:

All this speculation..... A minimal example and the answer would be simple. 

I have managed to make a minimal example that leaks. I will attach it here.

 

I have found this article on how to make a TDataModule reference counting written by Jeroen Pluimers https://wiert.me/2009/08/10/delphi-using-fastmm4-part-2-tdatamodule-descendants-exposing-interfaces-or-the-introduction-of-a-tinterfaceddatamodule/ and tried to implement it in MainDMSimulation in the example. Despite that, the example is still leaking.

 

I think is I am still doing something wrong when it comes to making TDataModule reference counting or in the way I resolve the interface in the Spring4D container (or maybe both).

 

Test.zip

[Anders Melander 1783]

1 hour ago, Alberto Paganini said:

TDataModule reference counting written by Jeroen Pluimers

That implementation is a bit over-engineered because it tries to handle mixing interface and object references. All you need to do reimplement _AddRef and _Release and stick to referencing the datamodule by the interface. Use TInterfacedObject as an example.

There was a thread here earlier this week on that exact topic but apparently it was deleted or maybe got lost when the server went down.

[Alberto Paganini 3]

Quote

All you need to do reimplement _AddRef and _Release and stick to referencing the datamodule by the interface. Use TInterfacedObject as an example.

I looked into TInterfacedObject again and implemented _AddRef and _Release in the same way, leaving out the remaining procedures, the example attached to this post is the updated minimum example. Unfortunately, I still have the same leaks.

 

Quote

There was a thread here earlier this week on that exact topic but apparently it was deleted or maybe got lost when the server went down.

I started that thread. Now I know why I cannot have access to it any longer.

Test2.zip

[Anders Melander 1783]

2 hours ago, Alberto Paganini said:

leaving out the remaining procedure

No. You need those too. They have a purpose.

 

I think you need to start with a much simpler test case. Does it leak if you  leave out all the Spring stuff and just do this:

var
  Test: IMainDMTEST;
begin
  Test:= TDMSimulation.Create(nil);
  Test := nil;
end;

 

[Alberto Paganini 3]

28 minutes ago, Anders Melander said:

No. You need those too. They have a purpose.

 

I think you need to start with a much simpler test case. Does it leak if you  leave out all the Spring stuff and just do this:

var
  Test: IMainDMTEST;
begin
  Test:= TDMSimulation.Create(nil);
  Test := nil;
end;

 

Sorry I didn't explain properly.  _AddRef and _Release are implemented in my example. It was the other procedures from Jeroen's example that have been left out as you advised.

If I create the object as per your example above then there is no leak. I suppose the issue is in the way I resolve the interface in the Spring container.

[Anders Melander 1783]

34 minutes ago, Alberto Paganini said:

Sorry I didn't explain properly.  _AddRef and _Release are implemented in my example. It was the other procedures from Jeroen's example that have been left out as you advised.

Yes I understood that. What I mean is that you need the other methods from TInterfacedObject to protect against early release. Notably: AfterConstruction and NewInstance.

 

38 minutes ago, Alberto Paganini said:

I suppose the issue is in the way I resolve the interface in the Spring container

One step closer. This is why you should start with a minimal example so you can eliminate possible sources.

 

Next try to just register the interface/implementation in Spring (without any of the supporting code) and instantiate an instance of IMainDMTEST.

Make sure you release the instance before termination (like the unnecessary nilling in my example). You want to make sure Spring isn't holding on to a reference.

 

I can see from your code that you mark IMainDMTEST as a singleton. I don't use Spring but my guess is that Spring holds a reference so it can return the same instance (i.e. singleton) if asked for it. Try with and without AsSingleton.

[Alberto Paganini 3]

Quote

Yes I understood that. What I mean is that you need the other methods from TInterfacedObject to protect against early release. Notably: AfterConstruction and NewInstance.

Ok, I have also implemented QueryInterface, AfterConstruction, BeforeDestruction and NewInstance in my DataModule
 

Quote

Next try to just register the interface/implementation in Spring (without any of the supporting code) and instantiate an instance of IMainDMTEST.

In this case, there are no leaks in both cases, with our without .AsSingleton.

At this stage, I started implementing the other classes, one by one and .AsSingleton in my datamodule started making a difference when I arrived at the class TBetFairAPINGHorseFunctionSimulationBL

  TBetFairAPINGHorseFunctionSimulationBL = class(TInterfacedObject, ILBS_BetfairAPINGFunctionsBL)
  private
    FDM: IMainDMTEST;
  public
    constructor Create(aDM: IMainDMTEST); reintroduce;
    destructor Destroy; override;
  end;


constructor TBetFairAPINGHorseFunctionSimulationBL.Create(aDM: IMainDMTEST);
begin
  inherited create;
  FDM := aDM;
end;


destructor TBetFairAPINGHorseFunctionSimulationBL.Destroy;
begin
  FDM := nil;
  inherited;
end;

 

Quote

Make sure you release the instance before termination (like the unnecessary nilling in my example). You want to make sure Spring isn't holding on to a reference.

I have added TBetFairAPINGHorseFunctionSimulationBL.Destroy and set FDM=nil in there but this destructor is never executed, don't know why as this class inherits from TInterfacedObject?

As the last attempt, I have added a line at the end of the program to try to release the IMainDMTest at the very end of the application but the leak is still there.

var
  FScalpingBL: IScalpingBL;
  Test: IMainDMTEST;

begin
  RegisterTypes(GlobalContainer);
  Test := GlobalContainer.Resolve<IMainDMTEST>;
  FScalpingBL := globalcontainer.Resolve<IScalpingBL>;
  Test := nil;
end.

 

Quote

Try with and without AsSingleton.

If I remove .AsSingleton when I resolve the interface then I have no more leaks but I have to keep .AsSingleton because this class holds/manages data and several classes have access to it.

Edited July 5, 2020 by Alberto Paganini

[David Heffernan 2345]

Can't you make a minimal example? That's so important. I can't stress highly enough how important it is to make the example minimal. 

[Alberto Paganini 3]

9 minutes ago, David Heffernan said:

Can't you make a minimal example? That's so important. I can't stress highly enough how important it is to make the example minimal. 

Sure, here it is in attach.

I just wanted to avoid to attach a minimal example every time.

Test3.zip

[David Heffernan 2345]

I can't believe that spring's DI container requires all interfaces to have lifetime controlled by reference counting. I suspect that you've been given a dud steer with all this reference counting code that you've added to your data module.

 

@Stefan Glienke would be in a better place to give guide you. He also might be able to compile this code where I can't because I guess I'm using a different version of spring from you.

[Stefan Glienke 2002]

Solution:

Pass TRefCounting.True to AsSingleton because otherwise the container uses a very basic way to determine if the instance is reference counted or not and that is often false if it was implemented by yourself.

 

Explanation:

The container needs to know that in order to store the instance served as singleton and properly destroy it in the end:

- either by letting go the interface reference if it was reference counted

- or calling Free on the object reference if it was not reference counted

 

The issue has been open for a while now and I have a few ideas but it did not make it to the top of my priority list yet:

https://bitbucket.org/sglienke/spring4d/issues/237

 

Alternative solution:

If you are using the instance as singleton why implement reference counting in the first place - it makes no sense.

 

By the way:

Stop using so many delegates if all you do is resolve all the ctor parameters yourself - that defeats the purpose of using a DI container.

If you want to force the DI container to use a certain ctor then put the [Inject] attribute over the ctor- then you will get a resolve exception when it could not provide all parameters.

That might be important because by default the container falls back to a less greedy ctor if it cannot provide all parameters (you can configure that behavior) which sometimes lead to defects at it always ultimately falls back to TObject.Create (RTTI cannot see that it might not be visible because information about method overloading is not available)

If you want to use a certainly named service to be injected then put the [Inject(<servicename>)] attribute on the specific ctor parameter.

Another way would be to use InjectConstructor in the registration and provide the parameternames (at this time you have to specify all parameters - if you want to use the default service then pass TValue.Empty or nil) - I am working on being able to provide value/servicename for individual ctor parameters because that also makes things robust agains refactorings.

 

The code you wrote actually serves no purpose at all - it would be shorter and faster if you used pure DI instead because with all the DelegateTo calls you basically did just that.

If you put a parameterless ctor into your DM class which just calls inherited Create(nil); then you can get rid of all DelegateTo calls in your registration and just let the container do its job.

Edited July 5, 2020 by Stefan Glienke