Shouldn't calling Free on an object reference set to nil throw an Access Violation every time it is called?

Question

I'm getting access violations from the unit DBXCommon.pas (in Delphi XE). When I look at the code I see things like the following (at the exclamation marks):

function TDBXConnectionFactory.GetConnection(const DBXContext: TDBXContext;
  const ConnectionProperties: TDBXProperties): TDBXConnection;
var
  ConnectionBuilder:  TDBXConnectionBuilder;
  DelegatePath:       TDBXDelegateItem;
  Connection:         TDBXConnection;
  CombinedProperties: TDBXProperties;
begin
  //...
  ConnectionBuilder := TDBXConnectionBuilder.Create;
  Connection        := nil;
  try
    //..lots of setting ConnectionBuilder properties
    ConnectionBuilder.FInputPassword := CombinedProperties[TDBXPropertyNames.Password];
    Connection := ConnectionBuilder.CreateConnection;
    Connection.Open;
    Result     := Connection;
!!  Connection := nil;
  finally
!!  Connection.Free;
    ConnectionBuilder.Free;
  end;
end;

But I see constructs like this (first assign Nil, then a Free) much more in DBXCommon.pas. Is this some construct I do not know, or is this really causing access violation every time this piece of code is called?

Dupe - http://stackoverflow.com/questions/8548843/why-should-i-not-use-if-assigned-before-using-or-freeing-things — OnTheFly, Dec 21 '11 at 17:19

score 16 · Accepted Answer · answered Dec 21 '11 at 14:25

Calling Free on a null reference is always safe. Go look in the implementation of TObject.Free to see why.

This code is an example of a factory function. Its job is to create a new instance of a class, but if it fails, it needs to make sure it doesn't leak a half-created instance when it throws an exception, so it calls Free. When it's sure it's going to succeed, it transfers ownership of the result to the caller. It still calls Free, but if it's already transfered ownership, then it ends up calling Free on a null reference, and there's no harm done. This code is what transfers ownership:

Result := Connection;
Connection := nil;

The way I would write a factory function would do away with the separate Connection variable. I'd construct the result directly in Result, but free it if there were an exception, like this:

function TDBXConnectionFactory.GetConnection(const DBXContext: TDBXContext;
  const ConnectionProperties: TDBXProperties): TDBXConnection;
var
  ConnectionBuilder:  TDBXConnectionBuilder;
  DelegatePath:       TDBXDelegateItem;
  Connection:         TDBXConnection;
  CombinedProperties: TDBXProperties;
begin
  //...
  ConnectionBuilder := TDBXConnectionBuilder.Create;
  try
    //..lots of setting ConnectionBuilder properties
    ConnectionBuilder.FInputPassword := CombinedProperties[TDBXPropertyNames.Password];
    Result := ConnectionBuilder.CreateConnection;
    try
      Result.Open;
    except
      Result.Free;
      raise;
    end;
  finally
    ConnectionBuilder.Free;
  end;
end;

That has the same effect.

Thanks, somewhere in the back of my head I knew this. However, writing it as you show would be clearer for the untrained eye. — Geerten, Dec 21 '11 at 14:44
+1 this is the idiomatic way to write a function that returns a new object — David Heffernan, Dec 21 '11 at 15:02
I agree with David, except that if you do not re-raise exceptions, you should set the Result to nil. It's something we do in performance critical code where checking for a non-nil result is faster than raising and handling an exception... — Marjan Venema, Dec 21 '11 at 19:30
@Marjan, if you do not re-raise exceptions, then you must be handling them. That's fine. Fixing the problem that caused an exception to be raised is a very good thing to do. But most code is not in a position to fix all (or even *any*) of the problems that might lead to exceptions. Any code that catches an exception but neither fixes the problem nor re-raises it is wrong. — Rob Kennedy, Dec 21 '11 at 20:59
@RobKennedy: No arguments there from me :). The code I am talking about is indeed set up to go on as best it can, try alternatives, or roll back whatever it started. It's a server pulling 80+ GigaBytes in memory and with a load time of up to a couple of hours we don't want it to fall over just because something happens not to be available... — Marjan Venema, Dec 21 '11 at 21:22

ain · Answer 2 · 2011-12-21T14:34:38.083

7

It is safe to call Free on nil reference as it's implementation checks for Self <> nil before calling Destroy. See Allen Bauer's explanation in Embarcadero forum why TObject.Free was introduced. I include only the relevant quote here:

The sole reason for introducing the non-virtual Free method on TObject, was for use in destructors as a simple shorthand for:

if FField <> nil then
  FField.Destroy;

edited Dec 21 '11 at 14:34

answered Dec 21 '11 at 14:24

ain

22,394
3
54
74

2

haven't learned the lesson by now? people should stop linking to Embarcadero webpages. they are there for only 2-3 years. then... pufffff. gone :) :) – Gabriel Mar 25 '19 at 17:55

score 4 · Answer 3 · answered Dec 21 '11 at 14:24

4

TObject.Free is basically implemented as if Self <> nil then Destroy, so the code above should not raise any exception.

answered Dec 21 '11 at 14:24

Andreas Rejbrand

105,602
8
282
384

All the other answers are too complex and deviate too much from the point. I think this answer should be the accepted one. – Gabriel Jun 01 '22 at 09:56

Shouldn't calling Free on an object reference set to nil throw an Access Violation every time it is called?

3 Answers3

Linked