Take care of CONST and interface parameters!

News 17 comments

As one of the good guys (Agustín Ortu) recently pointed about one very tricky thing about const parameters and interfaces I decided that it is worth to share that with all of you.

So imagine you have the following code:

IHandler = interface ...

THandler = class(TInterfacedObject, IHandler) ...

procedure DoSomething(CONST AHandler: IHandler);
begin
  { No code accesses the AHandler }
end;
...
DoSomething(THandler.Create)

Looks safe, isn’t it?

NO. There is a potential memory leak. because if no code addresses the AHandler inside of the routine DoSomething you will get a leak of the THandler instance!

Why this happens:

  1. THandler is constructed – it’s just plain object of TInterfacedObject type;
  2. It is passed as pointer to the DoSomething function (as it is const). So no reference is added and the FRecCount = 0;
  3. So now if no code is accessing the AHandler as IHandler type the reference count will not take place and the object is leaked!

How to avoid:

  1. Define the parameter as non-const (that’s the best – and don’t cry about “redundant” calls to _AddRef, _Release) – as then once the code enters function the reference will be added and once scope is left the object will be released and there will be no memory leak;
  2. Always assign the first the reference to the local variable and then pass it to the parameter;
  3. Write DoSomething(THandler.Create as IHandler);
  4. Make a constructor function like NewHandler: IHandler and make the explicit creation there (like NewXMLDocument function).

Which one to choose is your business.

But you need to take care when you choose to use the const prefix for the parameter of interface type and consider reading about parameters!

Have a nice Friday!

17 comments

      1. You can write a simple factory method. I mean an anonymous function that returns the interface. That way you can always use const for the interface. It’s not only about perf on ._AddRef; const says a lot to me. And also, you don’t need to rely on specific implementation details and check if the implementation is storing or not the reference. Also, this is a bug, i’m sure its been reported and should be fixed. If someday got fixed, you’re code just stays fine.

        Btw, i didn’t know about option 3. Once again, thanks Stefan!

        Like

  4. Given that THandler.Create as IHandler (which stores the IHandler in a hidden local temp variable, which causes AddRef/Release) works, I would consider to classify this as a compiler bug. Even without using the explicit “as IHandler”, the compiler should be storing the IHandler derived from the THandler.Create in a local temp before calling DoSomething.

    Liked by 2 people

    Reply

      1. I disagree. This is a compiler bug.

        When the compiler performs an *implicit cast* from an object reference to an interface reference, if not assigned to an explicit variable, it should always store the result of the cast in a (hidden) temp variable.

        In exactly the same way as it already does when an *explicit cast* is being performed.

        DoSomething(THandler.Create)

        should result in identical generated code as

        DoSomething(THandler.Create as IHandler)

        Delphi interfaces are COM interfaces. So ignoring any delphi specific issues on top of it, “The Rules of the Component Object Model” apply.

        https://msdn.microsoft.com/en-us/library/ms810016.aspx

        Under “Reference-Counting Rules” we find:

        Rule 1c: New pointers synthesized out of “thin air.” A function that synthesizes an interface pointer using special internal knowledge, rather than obtaining it from some other source, must do an initial AddRef on the newly synthesized pointer. Important examples of such routines include instance creation routines, implementations of IUnknown::QueryInterface, and so on.

        Independent of the fact that the call to DoSomething does not involve increasing the reference count of the passed interface (because of the const), this rule applies to the step of getting from a THandler object pointer to an IHandler interface pointer, which in this particular case is done by the compiler “using special internal knowledge” (specifically adding x to the instance pointer, which leads to the interface pointer for this object instance that was filled in during the creation of the object instance by the RTL code).

        The rules of COM say that before anything else is done with such a synthesized pointer (specifically calling DoSomething in this case), AddRef needs to be called.

        The compiler does that for an explicit cast. It forgets to do it for an implicit cast.

        This is a compiler bug.

        Liked by 2 people

      3. Unfortunately, I agree with you regarding the likelihood of it being fixed.

        As it stands right now, a memory leak if DoSomething doesn’t touch aHandler is actually just a minor issue.

        Much worse is:

        procedure DoSomething(CONST AHandler: IHandler);
        begin
        DoSomethingElse(aHandler); // not passed as const -> calls AddRef/Release
        aHandler.Fubar; // AV because the THandler behind the IHandler has been freed above
        end;

        Liked by 2 people

  5. At my firm (roughly 50 Delphi programmers) we have a large number of libraries where we use interfaces mainly for reference counting. We always provide a factory function for construction of interfaced objects. For example, instead of using TMyHandler.Create we would make a function NewMyHandler: IMyHandler; and then hide the TMyHandler class in the implementation section of the unit, if possible, or at the least make the constructor private or protected.

    This is basically the same work-around to the bug as the explicit cast, except that it can’t be avoided. Our whole staff is used to this pattern.

    Liked by 1 person

    Reply

    1. But the topic is not about pattern or factory. Everyone knows the patterns but there is the talk about basic language feature… Patterns is another topic. And yes sure we also use the factories, but this is different topic.

      Like

      Reply

      1. I’m sorry you find it an inappropriate comment, but I disagree. You mention 3 possible “patterns” in your “how to avoid” section at the end of the article, but fail to mention this one, which I and my team have found to be the most effective (as you don’t have to remember to do use it at the point of call but you still get the performance benefit of avoiding unnecessary AddRef/Release calls).

        Also, I’m not sure “everyone knows the patterns” because the reason to use a constructor function instead of using the constructor directly is simply to avoid this bug. If “everyone knows the patterns” then everyone is already aware of the bug and there was no need for the article, which is clearly not the case ;). It’s a great article and I hope it is the top result for relevant searches.

        Liked by 2 people

Leave a Reply