Should Dispose() ever create new instances of objects?

Question

Using C#.NET 4.0

My company's application makes use of a resource locker to keep records from being edited simultaneously. We use the database to store the start time of a lock as well as the user who acquired the lock. This has led to the following (strange?) implementation of dispose on the resource locker, which happens to be called from a destructor:

protected virtual void Dispose(bool disposing)
        {
            lock (this)
            {
                if (lockid.HasValue)
                {
                    this.RefreshDataButtonAction = null;
                    this.ReadOnlyButtonAction = null;

                try
                {
                    **Dictionary<string, object> parameters = new Dictionary<string, object>();
                    parameters.Add("@lockID", lockid.Value);
                    parameters.Add("@readsToDelete", null);
                    Object returnObject = dbio2.ExecuteScalar("usp_DeleteResourceLockReads", parameters);**

                    lockid = null;
                }
                catch (Exception ex)
                {
                    Logger.WriteError("ResourceLockingController", "DeleteResourceLocks", ex);
                }
                finally
                {
                    ((IDisposable)_staleResourcesForm).Dispose();
                    _staleResourcesForm = null;
                }
            }
        }
    }

I am concerned about the bolded section we because have been logging strange "Handle is not initialized" exceptions from the database call. I read elsewhere that it is not safe to create new objects during Finalize(), but does the same rule apply to dispose()? Are there any possible side effects that accompany creating new objects during Dispose()?

Where did you read that it's not safe to create new objects in a finalizer? — Jon Skeet, Oct 17 '13 at 17:16
that would be very cool if SO let us bold the line(s) we have the issue on — Jonesopolis, Oct 17 '13 at 17:17
Have you disposed of `dbio2` already at that point in time? That could be why it's not working. — Servy, Oct 17 '13 at 17:19
@JonSkeet I got the Finalize() bit from the top answer in this thread: http://stackoverflow.com/questions/11348987 — abarger, Oct 17 '13 at 17:22
@FolksymAndrew: That's talking about referring to other existing objects, not creating new ones. — Jon Skeet, Oct 17 '13 at 17:25

score 1 · Answer 1 · answered Oct 17 '13 at 17:18

1

Dispose is just a method, like any other method. There are some conventions about things that it should/shouldn't do, but there's nothing from the system's perspective that is wrong with creating objects in a Dispose call.

Making a DB call is a bit concerning to be personally; I wouldn't expect such an expensive and error prone activity to be called in a Dispose method, but that's more of a convention/expectation. The system won't have a problem with that.

answered Oct 17 '13 at 17:18

Servy

202,030
26
332
449

1

Dispose in its recommended implementation is not actually "like any other method" - half of it (under `disposing==false`) is executed in finalizer where all managed sub-components may be gone/disposed, or at very least behave strangely due to execution on finalizer thread. The other half is indeed normal method. – Alexei Levenkov Oct 17 '13 at 17:29
1

@AlexeiLevenkov: Only 1% or less of public classes that implement `IDisposable` should have finalizers (or C# "destructors"). In the vast majority of cases, finalization logic should be confined to privately-held instances of classes whose purpose centers around finalization (e.g. `SafeHandle`). The recommended `IDisposable` pattern may have seemed like a good idea at the time, but having classes combine managed and unmanaged resources is generally a bad idea. Finalization code for a class should generally be similar to the managed cleanup (other than error handling), or else not exist. – supercat Apr 21 '14 at 20:42

score 1 · Answer 2 · answered Oct 17 '13 at 17:41

Yes, but I would not do so unless the object created is within the local scope of the method. IDisposable is an advertisement that this class has some resource (often an unmanaged resource) that should be freed when the object is no longer used. If your Dispose is being called by your finializer (ie you are not calling the destructor directly, but waiting for the GC to do it) it can be an indication that you should be calling it earlier. You never know when the C# destructor will run, so you may be unnecessarily tying up that resource. It could also be in indication that your class doesn't need to implement IDisposable.

In your case your are using object dbio2 which I assume represents your DB connection. However, since this is called from the destructor how do you know if your connection is still valid? You destructor could an hour after your connection has been lost. You should try to ensure that this Dispose is called while you know the dbio2 object is still in scope.

score 1 · Accepted Answer · answered Oct 17 '13 at 18:23

which happens to be called from a destructor

That's the real problem. You cannot assume that the *dbio2" object hasn't been finalized itself. Finalization order is not deterministic in .NET. The outcome would look much like you describe, an internal handle used by the dbase provider will have been released so a "Handle is not initialized" exception is expected. Or the dbio2 object was simply already disposed.

This is especially likely to go wrong at program exit. You'll then also have problem when the 2 second timeout for the finalizer thread, a dbase operation can easily take more.

You simply cannot rely on a finalizer to do this for you. You must check the disposing argument and not call the dbio2.ExecuteScalar() method when it is false. Which probably ends the usefulness of the destructor as well.

Should Dispose() ever create new instances of objects?

3 Answers3