Is using the null conditional operator in a destructor bad practice?

Question

I have a class with a destructor that I get a null reference exception from because the variable I destroy is sometimes null.

Is this an appropriate use of the null conditional operator to be in the destructor?

I'm not even sure if this is an appropriate use of a destructor itself since it is not used to dispose of the actual object it's called on but rather a variable of it.

~clsSAPSettings()
{

    mtbTemp?.Close();
}

This code was converted from VB6 so I'm trying to figure out how to handle this issue. Any information welcome.

Edit: The class mtbTemp belongs to implements IDisposable but doesn't have a finaliser/desctructor. It simply closes a connection used in an ORM model.

For anyone after a detailed explanation I found a great answer, Proper use of the IDisposable interface, it goes into detail about the use of finalizer and how garbage collection actually works.

If the type of `mtbTemp` is another managed type then this code is wrong. It belongs inside an `IDisposable.Dispose`, not a finalizer — Damien_The_Unbeliever, Jun 15 '18 at 08:56
First of all, the null conditional operator is completely safe to use, the question is why you're calling Close on a different object in the destructor. You shouldn't do this at all. Instead you should implement the dispose pattern correctly. — Lasse V. Karlsen, Jun 15 '18 at 08:56
Isn't that exactly what a destructor's for, closing down / disposing the resources that an object is using? I think your usage seems completely valid. — Oystein, Jun 15 '18 at 08:56
No, the destructor is for disposing of **unmanaged resources**, ***not*** managed resources. If you have an `IntPtr` or something similar that is a handle to some unmanaged object, the destructor is one place to close that, in addition to the disposable pattern. You should **not** call into other objects for this kind of thing. — Lasse V. Karlsen, Jun 15 '18 at 08:57
@Oystein - yes, in C++, no in C#. Question is tagged [tag:c#] — Damien_The_Unbeliever, Jun 15 '18 at 08:57
If the `mtbTemp` object needs to do something in the finalizer, it should have its own destructor, it's not the destructor of `clsSAPSettings` job to do this. — Lasse V. Karlsen, Jun 15 '18 at 08:58
@LasseVågsætherKarlsen Yes, couldn't this be the case here? Let's say you have a CommunicationHandler class, with a SerialPort implementation. Wouldn't the destructor be a proper place to make sure the SerialPort object is closed/disposed? — Oystein, Jun 15 '18 at 09:00
The destructor of the SerialPort class, correct, not the destructor of the CommunicationHandler. — Lasse V. Karlsen, Jun 15 '18 at 09:04
http://blog.stephencleary.com/2009/08/how-to-implement-idisposable-and.html — TheGeneral, Jun 15 '18 at 09:06
The class mtbTemp belongs to does implement IDisposable but doesn't have a finaliser/desctructor. It simply closes a connection used in an ORM model. (will add to question) — George K, Jun 15 '18 at 09:12

Dmitry Bychenko · Accepted Answer · 2018-06-15T12:17:29.177

5

Please do not use any fields of reference types in the finalizer: the order in which GC (Garbage Collector) collects them is unpredictable and that's why

 ~clsSAPSettings()
 {
     mtbTemp?.Close();
 }

code can well be performed by GC as followes:

Collect mtbTemp instance
Start collecting this instance:
Call ~clsSAPSettings()
Call mtbTemp?.Close(); i.e. call a method of the collected (destroyed) instance

and you'll have an unstable hard to find error. It seems that you are looking for a IDisposable interface:

 public class clsSAPSettings: IDisposable {
   private MyTemp mtbTemp;

   ... 

   protected virtual void Dispose(bool disposing) {
     if (disposing) { 
        mtbTemp?.Close();

        GC.SuppressFinalize(this); 
     }
   }  

   public void Dispose() {
     Dispose(true);
   }

   //TODO: do you really want finalizer?
   ~clsSAPSettings() {
     Dispose(false);
   } 
 }

edited Jun 15 '18 at 12:17

answered Jun 15 '18 at 09:04

Dmitry Bychenko

180,369
20
160
215

observation: there's no point suppressing the finalizer if you're already in the finalizer, so `GC.SuppressFinalize(this)` could move to the `if (disposing) {...}` branch - which then exposes that we're not actually doing *anything* in the finalizer case, which makes it very clear that we don't need a finalizer – Marc Gravell Jun 15 '18 at 09:10
@Marc Gravell: Thank you! Nice catch. `GC.SuppressFinalize(this)` indeed should be within `if (disposing) {...}` – Dmitry Bychenko Jun 15 '18 at 09:13
Thanks for you answer it's quite informative. I have a question, why do you not trigger the disposal (close()) of mtbTemp from the finalizer? Shouldn't that object (the conntection) be closed when this object is destroyed? – George K Jun 15 '18 at 09:42
@George K: I call `Dispose(false);` (not `true`) from the *finalizer* because `mtbTemp` instance can well be *collected* (i.e. destroyed) and that's why we can't call any method of such an instance. Please, have a look at such scenario in my answer ("1. Collect mtbTemp instance ..."). – Dmitry Bychenko Jun 15 '18 at 09:47
@DmitryBychenko So you mean that mtbTemp could be disposed of another way somehow (listed in your answer) hence we can't manually collect it now? Or that it will be collected as part of the destruction of the object it belongs to? I'm still unsure as to why we can't manually collect it since it's not being used anywhere else (only belongs to that object) Thanks Dmitry – George K Jun 15 '18 at 09:54
1

@George K: GC forms the collection of objects to destroy, then destroys them in arbitrary order. That's why we can't be sure in the finalizer that `mtbTemp` has not be destroyed. – Dmitry Bychenko Jun 15 '18 at 09:55

Marc Gravell · Answer 2 · 2018-06-15T09:11:53.363

Consider:

~clsSAPSettings()
{    
    mtbTemp?.Close();
}

The problem here isn't the null conditional usage. That doesn't by itself present any issues.

The biggest problem is that in a finalizer you should not touch any other object. When the finalizer fires, your object is toast. You no longer have any guarantees about the life of mtbTemp, including whether or not it has already been garbage collected, so you should not touch it. It might work; it might cause a temporary resurrection, or it might crash horribly.

The correct place to do something like this is in IDisposable.Dispose. In the Dispose method, that would be absolutely fine:

public void Dispose() // where your class : IDisposable
{    
    mtbTemp?.Close();
    mtbTemp = null;
}

You probably don't need a finalizer at all. They are incredibly rare.

Patrick Hofman · Answer 3 · 2018-06-15T09:05:19.207

3

When you are closing streams or other unmanaged objects, you should use the Dispose pattern, rather than a destructor. You never know when that destructor will fire.

Regarding the use of the null conditional operator in a destructor: I see no problem with the operator itself. I do with referencing other objects that might already been destructed or in destruction.

edited Jun 15 '18 at 09:05

answered Jun 15 '18 at 08:56

Patrick Hofman

153,850
22
249
325

It's only "safe" insofar you're touching objects that don't have destructors but you have to be **super certain that this will always be true**. In general, never touch any *objects*. – Lasse V. Karlsen Jun 15 '18 at 09:05

Is using the null conditional operator in a destructor bad practice?

3 Answers3