The Wiert Corner – irregular stream of stuff

Jeroen W. Pluimers on .NET, C#, Delphi, databases, and personal interests

  • My badges

  • Twitter Updates

  • My Flickr Stream

  • Pages

  • All categories

  • Enter your email address to subscribe to this blog and receive notifications of new posts by email.

    Join 1,427 other followers

Another case against FreeAndNil

Posted by jpluimers on 2017/12/21

I started this post as “A case against FreeAndNil” but soon found out about [WayBack/Archive.isA case against FreeAndNil – Community Blogs – Embarcadero Community.

Recently I was refactoring a bunch of code that was classed based to become interface based.

The old code had a lot of references to classes like this:

FMyField: TMyClass;

The compiler had detected those as compiler errors so now they were like this:

FMyField: IMyInterface;

But the compiler would still happily compile code (for instance in destructors) containing:

FreeAndNil(FMyField);

The only way that FreeAndNil can be implemented is with an untyped var parameter:

procedure FreeAndNil(var Obj);
var
  Temp: TObject;
begin
  Temp := TObject(Obj);
  Pointer(Obj) := nil;
  Temp.Free;
end;

This var construct makes it typeless: the method dutifully accepts anything even something that isn’t a TObject. And there is hardly anyw any (cheap) way to detect it’s actually a TObject (as it likekly won’t have RTTI).

What happens when you call it with an interface as parameter is that the hard cast will execute a “Free” method that might not be a method at all. Or is a method that does something totally different, expects more parameters (hello stack-underflow!) or has other harmfull side effects.

So contrary to the past – where most people would prefer FreeAndNil – I’m back to the below code in class based situations:

FMyField.Free(); FMyField := nil;

This looks like a strange but it has the visual clue that both are being executed and belong together.

Edit: the above is why I wrote a generic FreeAndNil inside a helper that limits itself to TObject references. The gist is below. This is the declaration:

class procedure FreeAndNil<T: class>(var Value: T); static;

Some future enhancements:

Blast from the past which is how I used to do it:

–jeroen

11 Responses to “Another case against FreeAndNil”

  1. rvelthuis said

    FWIW, in which way is your Cast better than a simple as? ISTM it does the same and does not require a record helper. And why do you assign the exception to a variable lException before you raise it?

    • jpluimers said

      It is better in the sense that you get more context on when things go wrong.

      That’s also why lException is there: if you put a breakpoint, you can immediately see the context spelled out for you before the exception even happens.

      In practice that is a real life saver, especially when you land a project maintaining sources that are in part not of your own quality standards.

  2. Waaaaaah! Do not make it “safer” to use FreeAndNil. Simpy advise people to avoid it as much as possible.Code that needs FreeAndNil is most of the time smelly. FreeAndNil is not a safety belt. It is something that looks like a safety belt.

    • jpluimers said

      I think this improved version is indeed a safety belt. It at least saved me from quite a lot of accidents both in the form of wrongly freed interfaced references and ensuring a nil pointer gets the proper exception when dereferenced (instead of some hard to track down memory overwrite).

      It still means you need to know what memory and pointer operations actually do, but it makes it a lot more manageable.

      Of course your mileage might very.

      One thing I am contemplating about is to call it NilAndFree as the implementation in the RTL System.SysUtils.pas is very similar to https://github.com/synopse/LVCL/blob/master/SysUtils.pas#L2970

      procedure FreeAndNil(var Obj);
      var Temp: TObject;
      begin
        Temp := TObject(Obj);
        Pointer(Obj) := nil;
        Temp.Free;
      end;
      • rvelthuis said

        Anything that promotes the use of FreeAndNil is a problem. FreeAndNil is most of the time used for the wrong purpose. If FreeAndNil is needed to make code “safer”, usually something much more fundamentally is wrong. If FreeAndNil solves, or at least seems to solve an occurring problem, it is never investigated and never really solved. So instead of making it “safer” to use FreeAndNil, rather advise people to avoid it like the plague, even if it seems to solve something.

        • jpluimers said

          For the same reason, you can argue that setting anything to nil would be void with the exception of breaking circular references.

          The practice is that code has bugs. Most of them are not even fundamental issues, but small glitches. Regardless of the classification, unless you have unlimited time, you cannot prevent all of these bugs. You can hoever make sure that when they happen, you have enough pointers (pun intended) to find the cause as quickly as possible.

          That’s what where nilling pointers and therefore a good version of FreeAndNil can be very helpful.

  3. Uwe Raabe said

    May be it is nit-picking, but

    FreeAndNil(instance);

    is not the same as

    instance.Free; instance := nil;

    There might be cases where the order of the call to Free and setting the instance to nil may be important. Especially when you have to admit that you may not be aware of the consequences in the code you are working on.

  4. Adam Lipovský said

    I am sorry, but I didn’t catch the point of this article. When I use FreeAndNil, I HAVE TO KNOW what I am doing – what object I am freeing. If I work with interfaces, I do not free them at all – they are reference counted and so they free on they own.

    FreeAndNil() is only for cases, when I have repeatedly used reference which I have to test against nil – Assigned(). In case of locals, instance.Free is well enough.

    • jpluimers said

      You hit the nail on the had with

      I HAVE TO KNOW what I am doing

      Often when maintaining code, you do not always know the consequences of a refactoring from TObject references to interface references:

      • The compiler doesn’t warn you in all cases (especially not teh FreeAndNil case
      • The IDE “find all references” fails to actually find all references

      The underlying problem is that FreeAndNil has an untyped var parameter.

      So you can call it with a reference to an interface, especially existing code that you have already refactored the reference from a TObject to an interface reference.

      If you do that, it regards it as a TObject, then sets the reference to nil without realising it is in fact an interface so the FRefCount is not decremented, then finally frees the instance.

      This causes all sorts of problems in two cases:

      1. if you still have references around (bad as you get access violations or similar exceptions)
      2. on classes that actively check if the FRefCount is zero before freeing (still bad: even though usually you get a descriptive exception, but often with a hard to find stack trace)

      I just found back some code I threw together a while ago with a more typesafe FreeAndNil: https://gist.github.com/jpluimers/31a036b7c91bc121c422fcb1a0adbd39

    • rvelthuis said

      If you know what you are doing, you dont need FreeAndNil. I probably never needed/used it, in all the years it exists.

Leave a Reply

Fill in your details below or click an icon to log in:

WordPress.com Logo

You are commenting using your WordPress.com account. Log Out /  Change )

Google+ photo

You are commenting using your Google+ account. Log Out /  Change )

Twitter picture

You are commenting using your Twitter account. Log Out /  Change )

Facebook photo

You are commenting using your Facebook account. Log Out /  Change )

Connecting to %s

 
%d bloggers like this: