Exception in constructor of class taking ownership of an object instance

[dummzeuch 1505]

Given a class like this:

type
  TMyClass = class
  private
    FSomeInstance: TSomeOtherClass;
  public
    constructor Create(_SomeInstance: TSomeOtherClass);
    destructor Destroy; override;
  end;

[...]

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  FSomeInstance := _SomeInstance;
  // other code here
end;

destructor TMyClass.Destroy;
begin
  FSomeInstance.Free;
  inherited;
end;

Note: The class takes ownership of the TSomeOtherClass instance passed to it in the constructor. It frees it in its destructor.

 

How would you write the code constructing it? My current code looks like this:

SomeInstance := TSomeOtherClass.Create;
try
  MyInstance := TMyClass.Create(SomeInstance);
  SomeInstance := nil;
finally
  FreeAndNil(SomeInstance)
end;

The try..finally  is there for handling the case where TMyClass.Create raises an exception, so there won't be a memory leak. But unfortunately the destructor is called even for partially created instances, so if the code after assigning FSomeInstance raises an exception, the instance of TSomeOtherClass would be freed twice.

 

The only safe way I can think of would be to change TMyClass.Create as follows:

constructor TMyClass.Create(var _SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  FSomeInstance := _SomeInstance;
  _SomeInstance := nil;
  // other code here
end;

If an exception gets raised in the inherited constructor (or in any other code before setting FSomeInstance) FSomeInstance would be nil, and _SomeInstance would be freed in the calling code. If it happens after setting _SomeInstance to nil, the destructor would free FSomeInstance and the calling code would not, becuse the var parameter was set to nil, which FreeAndNil (or even .Free) would detect.

 

Of course, I could use Interfaces and rely on reference counting.

Edited February 14, 2020 by dummzeuch

[Uwe Raabe 2057]

If the TMyClass.Destroy is even called when an exception is raised inside TMyClass.Create, you could as well assign FSomeInstance before calling inherited Create. That should make sure that FSomething is freed inside TMyClass.Destroy in all cases. That would make the outer try-finally obsolete.

 

Or are there situations where TMyClass.Destroy is not called?

[Dalija Prasnikar 1396]

21 minutes ago, Uwe Raabe said:

Or are there situations where TMyClass.Destroy is not called?

If allocating memory for the object instance fails then destructor is not called. 

 

I would probably use try..except block for releasing the object in case of out of memory error. Of course, that works only if constructing TMyClass instance itself is the only thing that can cause out of memory in that process.

 

But I don't really like that whole try...finally block with nil assignment. 

 

Cleaner approaches would include DI, interfaces, passing meta class to  TMyClass constructor and constructing TSomeOtherClass instance within the constructor, if the construction is too complex then I would use anonymous method instead and use it as factory. 

Edited February 14, 2020 by Dalija Prasnikar

[dummzeuch 1505]

56 minutes ago, Dalija Prasnikar said:

But I don't really like that whole try...finally block with nil assignment. 

 

Cleaner approaches would include DI, interfaces, passing meta class to  TMyClass constructor and constructing TSomeOtherClass instance within the constructor, if the construction is too complex then I would use anonymous method instead and use it as factory. 

How would DI (I guess you mean dependency injection) solve this problem? Can you please give an example?

[dummzeuch 1505]

1 hour ago, Uwe Raabe said:

If the TMyClass.Destroy is even called when an exception is raised inside TMyClass.Create, you could as well assign FSomeInstance before calling inherited Create. That should make sure that FSomething is freed inside TMyClass.Destroy in all cases. That would make the outer try-finally obsolete.

 

Or are there situations where TMyClass.Destroy is not called?

There are very rare cases where Destroy won't be called, but I would simply ignore them in this context.

 

Hm, assigning FSomeInstance before calling inherited Create. I am so used to call inherited Create as the first thing in the constructor, I didn't think of this. Sounds promising. Of course that only works if the destructor itself works as expected with a partially created object which - looking at the real world code this is about - I think doubtful. But that's a different problem I'll have to fix anyway.

[Dalija Prasnikar 1396]

6 minutes ago, dummzeuch said:

How would DI (I guess you mean dependency injection) solve this problem? Can you please give an example?

Same way like other approaches (besides interface) by moving construction of TSomeOtherClass instance to TMyClass constructor. Example would depend on DI framework used... I would look at Spring4D...

 

If code is used in places where out of memory is not recoverable error, then ignoring this potential leak is also viable approach in combination with assigning instance at beginning of constructor like @Uwe Raabe suggested

[Uwe Raabe 2057]

5 minutes ago, dummzeuch said:

I am so used to call inherited Create as the first thing in the constructor

That can fire back in some cases where the inherited constructor calls a virtual method that is overridden in TMyClass and relies on the existence of FSomeInstance. In that case it is required to assign it before calling inherited Create. This also holds true if FSomething is going to be created in TMyClass.Create. One has to inspect that carefully for each case. More than once I have been bitten by that when I introduced an override method.

 

 

[Stefan Glienke 2002]

1 hour ago, Uwe Raabe said:

That can fire back in some cases where the inherited constructor calls a virtual method that is overridden in TMyClass and relies on the existence of FSomeInstance. In that case it is required to assign it before calling inherited Create. This also holds true if FSomething is going to be created in TMyClass.Create. One has to inspect that carefully for each case. More than once I have been bitten by that when I introduced an override method.

I have been guilty of doing that myself but given this and other examples it shows why calling virtual methods from a ctor is considered a code smell by some people.

And from what I just learned C++ even refuses to do a virtual call as per https://isocpp.org/wiki/faq/strange-inheritance#calling-virtuals-from-ctors

 

And also "Injection Constructors should be simple"

Edited February 14, 2020 by Stefan Glienke

[Remy Lebeau 1394]

7 hours ago, dummzeuch said:

The only safe way I can think of would be to change TMyClass.Create as follows:

constructor TMyClass.Create(var _SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  FSomeInstance := _SomeInstance;
  _SomeInstance := nil;
  // other code here
end;

Another option would be to release ownership if the constructor fails, that way the caller retains ownership until the constructor is successful:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  FSomeInstance := _SomeInstance;
  try
    // other code here
  except
    FSomeInstance := nil;
    raise;
  end;
end;

Or, you could simply not take ownership until the very end after everything else is successful:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  // other code here
  FSomeInstance := _SomeInstance;
end;

If something needs to be called during construction that requires FSomeInstance, modify that something to let the constructor pass _SomeInstance to it as a parameter:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  // other code here
  DoSomethingWith(_SomeInstance);
  FSomeInstance := _SomeInstance;
end;

 

[dummzeuch 1505]

13 hours ago, Remy Lebeau said:

Or, you could simply not take ownership until the very end after everything else is successful:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  // other code here
  FSomeInstance := _SomeInstance;
end;

 

I don't see how assigning the field as the last operation in the constructor would solve the problem of duplicate destruction. Yes, if there is an exception, the class' destructor won't free SomeInstance and leave that for the finally block. But what if there is no exception? Then the try..finally around the construction will free SomeInstance and leave a stale pointer to an invalid instance inside the class.

Edited February 15, 2020 by dummzeuch
removed quote mess

[Fritzew 51]

14 minutes ago, dummzeuch said:

I don't see how assigning the field as the last operation in the constructor would solve the problem of duplicate destruction. Yes, if there is an exception, the class' destructor won't free SomeInstance and leave that for the finally block. But what if there is no exception? Then the try..finally around the construction will free SomeInstance and leave a stale pointer to an invalid instance inside the class.

Why using a finally here and not an except block?
Then you will only free it if there is a Exception.  

[dummzeuch 1505]

57 minutes ago, Fritzew said:

Why using a finally here and not an except block?
Then you will only free it if there is a Exception.  

You mean like this?

SomeInstance := TSomeOtherClass.Create;
try
  MyInstance := TMyClass.Create(SomeInstance);
except
  FreeAndNil(SomeInstance)
  raise;
end;

 

[Fritzew 51]

yes that was my thinking

[Fr0sT.Brutal 900]

With current construction

On 2/14/2020 at 4:29 PM, dummzeuch said:

 

SomeInstance := TSomeOtherClass.Create;
try
  MyInstance := TMyClass.Create(SomeInstance);
  SomeInstance := nil;
finally
  FreeAndNil(SomeInstance)
end;

 

 

 

you have to create an instance in the outer scope thus enlarging the code.

I'd move all processing to ctor, maybe this way

constructor
try
  inherited Create;
  ..
  Finst := Inst;
except
  Inst.Free;
  raise;
end;

or this

constructor
Finst := Inst;
inherited Create;
  ..

So that caller code would be just MyInstance := TMyClass.Create(TSomeOtherClass.Create);

 

[Dalija Prasnikar 1396]

19 minutes ago, Fr0sT.Brutal said:

With current construction

you have to create an instance in the outer scope thus enlarging the code.

 

Construction the instance in outer scope is indeed the problem here, but your code still constructs the instance in outer scope. In order to prevent all leaks, instance must be constructed from within the constructor because exception could be raised before constructor chain is called. In such case you still leak TSomeOtherClass instance.

[Fr0sT.Brutal 900]

1 minute ago, Dalija Prasnikar said:

 

Construction the instance in outer scope is indeed the problem here, but your code still constructs the instance in outer scope. In order to prevent all leaks, instance must be constructed from within the constructor because exception could be raised before constructor chain is called. In such case you still leak TSomeOtherClass instance.

Indeed. Everything depends on those classes, whether their creation could fail or not. Anyway the ability of exception in constructor seems to me like a hidden trap (not talking about alloc errors on low mem available - I suspect only a few specially prepared programs could survive this case).

[Dalija Prasnikar 1396]

25 minutes ago, Fr0sT.Brutal said:

Indeed. Everything depends on those classes, whether their creation could fail or not. Anyway the ability of exception in constructor seems to me like a hidden trap (not talking about alloc errors on low mem available - I suspect only a few specially prepared programs could survive this case).

Every instance construction can fail because of out of memory error.  But there is nothing special in surviving such exception. It only depends what you are doing at that time. Whether it will be recoverable error or not depends on the developer.

 

Also throwing exceptions during construction is not a trap... Delphi has mechanisms in place that allow full cleanup and recovery. That is all that matters. If we didn't have exception throwing construction, we would still have to deal with all kind of "exceptional" situations where you would have to be prepared for handling them. And it would not be any simpler than what we have now.

 

Basic problem in this particular case is non trivial ownership transfer, not the exception throwing constructors per-se. Solving ownership transfer solves the problem. Moving construction of dependent class to within owning class eliminates ownership transfer and solves core issue. (Another approach would be using reference counted classes and automatic memory management)

[dummzeuch 1505]

1 hour ago, Fr0sT.Brutal said:

the ability of exception in constructor seems to me like a hidden trap

As Dalija said: Raising exceptions in the constructor is possible and even normal (e.g. TFileStream.Create for a file that does not exist or maybe is only temporarily not available).

 

It's documented that the destructor must handle partly constructed objects.

 

Allen Bauer blogged about this in 2006 (aparently he has removed his About Me page from the blog, or did it never have one?)

 

But yes, many Delphi developers are not aware of this. I only recently (as in "a few years, maybe a decade ago") became aware of this issue and am still finding code that does not take this into account.

Edited February 17, 2020 by dummzeuch

[Fr0sT.Brutal 900]

2 hours ago, Dalija Prasnikar said:

Every instance construction can fail because of out of memory error.  But there is nothing special in surviving such exception. It only depends what you are doing at that time. Whether it will be recoverable error or not depends on the developer.

You're absolutely right but careful handling of OOM errors will be far from trivial and probably will not ever happen (considering OS virtual memory mechanism). And there's little an app could do in such case. Nevertheless, I'd read any resources on this subject with pleasure.

1 hour ago, dummzeuch said:

Raising exceptions in the constructor is possible and even normal (e.g. TFileStream.Create for a file that does not exist or maybe is only temporarily not available).

And I don't think this design is good 🙂

 

[Dalija Prasnikar 1396]

44 minutes ago, Fr0sT.Brutal said:

You're absolutely right but careful handling of OOM errors will be far from trivial and probably will not ever happen (considering OS virtual memory mechanism). And there's little an app could do in such case. Nevertheless, I'd read any resources on this subject with pleasure.

var
  Strings: TStringList;
begin
  Strings := TStringList.Create;
  try
    Strings.LoadFromFile('C:\SomehugeFile.txt');
  finally
    Strings.Free;
  end;
end;

Above is trivial example that can easily cause OOM error and it is also simple to handle and fully recoverable. You can easily wrap the whole thing with try...except block, show message to the user and application can merrily go on like nothing happened.

 

46 minutes ago, Fr0sT.Brutal said:

And I don't think this design is good 🙂

We disagree then. 

 

Maybe my imagination is at fault, but I have really hard time imagining how would solution with non-throwing constructors be simpler to handle and make code less complex and convoluted.

[Remy Lebeau 1394]

On 2/15/2020 at 1:13 AM, dummzeuch said:

I don't see how assigning the field as the last operation in the constructor would solve the problem of duplicate destruction. Yes, if there is an exception, the class' destructor won't free SomeInstance and leave that for the finally block. But what if there is no exception? Then the try..finally around the construction will free SomeInstance and leave a stale pointer to an invalid instance inside the class.

Um, do you see a try..finally in the example I provided?  No.  That example does not use one.  The previous example did.  2 different scenarios.

[dummzeuch 1505]

1 hour ago, Remy Lebeau said:

Um, do you see a try..finally in the example I provided?  No.  That example does not use one.  The previous example did.  2 different scenarios.

 

I was assuming that this constructor should replace the one in my original example, where it was called from within a try finally block:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  // other code here
  FSomeInstance := _SomeInstance;
end;

If that's not the case, you have a memory leak if the inherited Create fails.

[Remy Lebeau 1394]

5 hours ago, dummzeuch said:

I was assuming that this constructor should replace the one in my original example, where it was called from within a try finally block

Replace the try..finally with try..except instead, then it will be fine:

constructor TMyClass.Create(_SomeInstance: TSomeOtherClass);
begin
  inherited Create;
  // other code here
  // don't take ownership unless everything above is fine...
  FSomeInstance := _SomeInstance;
end;

...

MyInstance := nil;
try
  SomeInstance := TSomeOtherClass.Create;
  try
    MyInstance := TMyClass.Create(SomeInstance);
  except
    // ownership of SomeInstance is not taken, so free it now...
    SomeInstance.Free;
    raise;
  end;
  // ownership of SomeInstance is taken, will be freed when MyInstance is freed...
  ...
finally
  MyInstance.Free;
end;

 

Edited February 19, 2020 by Remy Lebeau