12

If you will look at the code of FreeAndNil procedure you will see:

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

What is the reason they assigning Nil to an object reference and only after this destroying it? Why not vice-versa?

Andrew
  • 3,696
  • 3
  • 40
  • 71
  • I would only assume to make sure that it becomes `nil` even if `Free` raised an exception of some kind. – Jerry Dodge Oct 29 '14 at 06:45
  • 1
    If you switch and the destructor raises an exception, you are left with a stale pointer. Wether you should use FreeAndNil, you might read up on it [here](http://www.nickhodges.com/post/Using-FreeAndNil.aspx) and draw your own conclusions. – Lieven Keersmaekers Oct 29 '14 at 06:45
  • To indicate other potential threads first that object no more exists. if you swap the order, between Free and nil assignment another thread could refer the invalid pointer. – pf1957 Oct 29 '14 at 06:47
  • 1
    @pf1957 - FreeAndNil is [not threadsafe](http://stackoverflow.com/questions/235033/why-are-delphi-objects-assigned-even-after-calling-free), no matter how you switch the code – Lieven Keersmaekers Oct 29 '14 at 06:51
  • 1
    @pf1957 In other words, that may be true, but should not be trusted because different threads shouldn't be accessing the same object without thread protection in the first place. An object *should* be "owned" by only one thread, and be synchronized for access from other threads. – Jerry Dodge Oct 29 '14 at 06:56
  • @LievenKeersmaekers Thanks for the interesting link. I never use `FreeAndNil` in my own code, but the old code is full of `FreeAndNil`s – Andrew Oct 29 '14 at 07:00
  • 2
    @Andrew That article is not the final say on the matter. There are other opinions. And don't read it as a blanket recommendation not to call FreeAndNil ever. Nick argues that if you write code his way, you don't need FreeAndNil. The argument only applies if you write the code the way he advocates. Not all code is written that way. – David Heffernan Oct 29 '14 at 07:29
  • 1
    If you want objects to be deleted safely while accessed by multiple threads then your best bet is to use interfaces. – Graymatter Oct 29 '14 at 07:29
  • From "Delphi in a Nutshell" about how to free and nil an object, `You should take care to set the variable to nil before freeing the object. If the destructor, or a method called from the destructor, refers to that variable, you usually want the variable to be nil to avoid any potential problems. ` FreeAndNil() does that for you. – LU RD Oct 29 '14 at 07:33
  • @LURD That's a bit scary. I can't picture a situation where you would need to refer to a variable that owns the object from inside the object. It feels messy and dangerous. I am not getting into the FreeAndNil discussion as I personally do use it from time to time. I am just concerned with how the situation could arise. – Graymatter Oct 29 '14 at 07:36
  • 2
    Not duplicate, but... http://stackoverflow.com/questions/8548843/why-should-i-not-use-if-assigned-before-using-or-freeing-things?rq=1 – Jerry Dodge Oct 29 '14 at 07:41
  • I guess I'll stay out of the discussion about the use of FreeAndNil. I don't code like Nick Hodges does, but I sure think it is obsolete. I am currently struggling to write an article that explains my way of memory management. – Rudy Velthuis Oct 29 '14 at 09:40
  • 2
    I've written about this before: http://www.cs.wisc.edu/~rkennedy/freeandnil – Rob Kennedy Oct 29 '14 at 12:12
  • @Graymatter, this scenario would likely not happen in normal code, but as Rob explains in his link (and Craig in his answer), there are cases when a global class variable could be addressed in a destructor chain by a subclass used inside the parent class. The VCL had some of these constructs, but I'm not sure nowadays. There is a state in some classes that tells if the class is within destruction. – LU RD Oct 30 '14 at 04:32

3 Answers3

16

I can think of two reasons for doing it this way round, neither of which seems at all compelling.

Reason 1: to guarantee that the reference is set to nil in case an exception is raised

The implementation achieves this. If the destructor raises, then the reference is still set to nil. Another way to do so would be with a finally block:

try
  TObject(Obj).Free;
finally
  TObject(Obj) := nil;
end;

The downside of this is performance. Particularly on x86 a try/finally is a little expensive. In such a fundamental routine it is prudent to avoid this expense.

Why do I find the desire to nil at all costs not to be compelling? Well, as soon as destructor start failing you may as well give up. You can no longer reason clearly about your program's state. You cannot tell what failed and what state your program is in. It is my view that the correct course of action in the face of a destructor that raises is to terminate the process.

Reason 2: to ensure that other threads can detect that the object is being destroyed

Again this is achieved but it is of no practical use. Yes you can test whether the reference is assigned or not. But then what? The other thread cannot call methods on the object without synchronization. All you could do is learn whether or not the object is alive. And if that is so, why would it matter if this status changed before or after the destructor runs?

So whilst I present this as a possible reason I cannot believe that anyone in Embarcadero was really swayed by this argument.

David Heffernan
  • 601,492
  • 42
  • 1,072
  • 1,490
  • For Reason 2 to hold, `FreeAndNil` by itself would have to be threadsafe and it isn't so I really only see one *(valid)* reason. – Lieven Keersmaekers Oct 29 '14 at 08:40
  • @LievenKeersmaekers I already say that in the answer. Without synchronization all that code in a different thread could do would be to test the reference was assigned or not. That would be safe. Whether the assignment to nil happened before or after the destructor does seem arbitrary though. Personally I see neither reason as being valid. – David Heffernan Oct 29 '14 at 08:49
  • @DavidHeffernan - Your voice carries lot of weight and it wasn't all clear to me from your answer. Now it is. – Lieven Keersmaekers Oct 29 '14 at 09:31
  • You are right to state than none of the two reasons is really compelling. – Arnaud Bouchez Jun 19 '20 at 11:12
11

There's a variation on David's second reason that is a little more compelling. Although one might argue that if it applies there are other problems that should be fixed.

Reason 3: to ensure event handlers on the same thread can detect that the object is being destroyed

Here's a concocted hypothetical example:

TOwner.HandleCallback;
begin
  if Assigned(FChild) then
    FChild.DoSomething;
end;

TChildClass.Destroy;
begin
  if Assigned(FOnCallback) then FOnCallback;
  inherited Destroy;
end;

Now if TOwner calls:

FChild.Free;
FChild := nil;

FChild will be asked to DoSomething in the middle of its destruction cycle. A certain recipe for disaster. The implementation of FreeAndNil neatly avoids this.

Yes you may argue that firing callback events during destruction is dangerous, but it does have its benefits. There are quite few examples in Delphi/VCL code. Especially if you expand the scope of concern to include calling polymorphic methods - which also put aspects of the destruction sequence outside of your control.

Now I must point out that I'm not aware of any specifc cases in Delphi library code where this problem could manifest. But there are some complex circular dependencies in parts of the VCL. So I wouldn't be surprised if changing the implementation to the more obvious choice in SysUtils leads to a few unpleasant surprises.

Community
  • 1
  • 1
Disillusioned
  • 14,635
  • 3
  • 43
  • 77
  • Seems pretty hypothetical. Since setting FChild to nil is private/protected to TChildClass then this would make sense only if FreeAndNil(FChild) is done within its own TChildClass.Destroy destructor. I don't see any actual example able to implement this hypothesis. If you have such a circular reference, then the same instance will own and call the FChild property. – Arnaud Bouchez Jun 19 '20 at 11:10
  • I have proposed some changes for FreeAndNil and var handling. Check out https://quality.embarcadero.com/browse/RSP-29714 and https://quality.embarcadero.com/browse/RSP-29716 and vote if it makes sense to you. Thx. – H.Hasenack Jun 19 '20 at 11:14
  • @H.Hasenack I don't see the very relevance of this RSP modification. Assigning a pointer is already atomic on an x86. It would make sense only if the method caller would also atomically exchange and own the pointer... but then without any reference counting it doesn't make any sense. Your code won't be safer, and would definitively be slower, since it would flush the CPU cache for no benefit. – Arnaud Bouchez Jun 19 '20 at 11:16
  • @Arnoud Bouchez In a multithreaded application, using the current implementation it could switch context between setting de original pointer to nil and calling temp.free . Hence TObject.Free might be called more than once for the same object by different threads. Of course it is little effort to create a ThreadSafeFreeAndNil alternative routine. BTW: I dont understand your remark about reference counting. I am unsure about the performance penalty, would have to test this. – H.Hasenack Jun 19 '20 at 11:31
  • @Arnoud Bouchez Wanna discuss this in a separate thread/MS teams? I've added a performance test to RSP-29714 comments. – H.Hasenack Jun 19 '20 at 12:13
1

The only thing that really bothered me for years is that FreeAndNil(var obj) lacks type safety. I know that due to the lack of adequate language support there was simply no way to do it right, but since we have generics for a while, here's a quick and simple tool that can make your life easier.

type
  TypeSafe = class sealed
  public
    class procedure FreeAndNil<T : class>( var obj : T);  static; inline;
  end;


class procedure TypeSafe.FreeAndNil<T>( var obj : T);
begin
  SysUtils.FreeAndNil( obj);
end;

If you add an overload for the normal FreeAndNil(var Obj) and mark it as deprecatedyou got a fair chance to find all the places where someone hands over an interface pointer to it -- and if the code base is only large enough you will find interesting things, believe me.

procedure FreeAndNil(var Obj); inline; deprecated 'use TypeSafe.FreeAndNil<T>() instead';

Note that you don't even have to specify the <T>since the compiler is smart enough to find out the right type for you. Just add the unit on top and change all

FreeAndNil(foobar);

in your source code into

TypeSafe.FreeAndNil(foobar);

and you're done.

JensG
  • 13,148
  • 4
  • 45
  • 55
  • Have you seen how Delphi 10.4 handle this better than your proposal using a const [ref] parameter? – fpiette Jun 19 '20 at 13:32
  • I implemented the generic solution using a helper class. So I dont (really) need the 'typesafe.' header. Downside is you can use only 1 helper class at any spot in your code AFAIK. IMO the const [ref] parameter isnt the best solution either. You dont expect a const paramter getting changed by the called routine. – H.Hasenack Jun 19 '20 at 13:46
  • While Delphi 10.4 [may handle it differently](http://docwiki.embarcadero.com/RADStudio/Sydney/en/New_features_and_customer_reported_issues_fixed_in_RAD_Studio_10.4#FreeAndNil), wether it is really better to sacrifice a const argument for type safety may be questionnable. Sure, in Delphi `const` has always been only sort of a "compiler hint", in comparison to the much more strict [const correctness](https://isocpp.org/wiki/faq/const-correctness) concept of C++. I could write volumes on what is wrong with `const` in Delphi. But I mean ... really? – JensG Jun 19 '20 at 15:32
  • I just recognized that the Syndey implemenation makes room for a whole new class of FreeAndNil errors. Because now you can FreeAndNil() a property. Previously the compiler would have told you that a const value cannot be assigned a var argument ... doh! – JensG Oct 14 '20 at 11:59