CODEWITHSUNDEEP
delphidelphi-xe
Geerten
Geerten

Reputation: 1057

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

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?

Upvotes: 9

Views: 3047

Answers (3)

ain
ain

Reputation: 22749

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;

Upvotes: 7

Rob Kennedy
Rob Kennedy

Reputation: 163247

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.

Upvotes: 16

Andreas Rejbrand
Andreas Rejbrand

Reputation: 108919

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

Upvotes: 4

Related Questions