SqlCommand.Dispose doesn't free managed object

[breyed]

SqlCommand.Dispose contains this code:

// release managed objects
_cachedMetaData = null;

This doesn't free the cached data, which is supposed to be the purpose of the Dispose(Boolean) overload when working with managed resources that don't implement IDisposable:

Managed objects that consume large amounts of memory or consume scarce resources. Freeing these objects explicitly in the Dispose method releases them faster than if they were reclaimed non-deterministically by the garbage collector.

By just setting _cachedMetaData to null, the resources are only reclaimed by the garbage collector, by which time the disposed SqlCommand object would also likely not be referenced, meaning that disposing the SqlCommand and setting _cachedMetaData to null is wasted effort.

---

This issue was observed as part of a discussion about this question: Is SqlCommand.Dispose() required if associated SqlConnection will be disposed?

[Wraith2]

The _SqlMetaDataSet type is fully managed and doesn't hold any scarce resources such as unmanaged handles or contended lock usage so it doesn't need any explicit lifetime management. It also can't really hold any user data (schema only) so it isn't something I'd regard as sensitive and subject to careful zeroing. All discarded managed memory is reclaimed for use by the GC, there's nothing special about the metadata cache that I can see.

The cached metadata is kept in case it is needed for faster reading of further rows from a returned datareader. In the case where any property of the command is changed causing it to be marked as dirty the cached metadata is dropped by nulling it, the disposable just follows this pattern. I don't see a problem with this behaviour.

A command object is typically used only once in a dispose block but you can easily reuse them. I've seen a program where a single command is used throughout a multi form maintenance program, and it worked.

[breyed]

Whereas marking the cache dirty on property change is required for correctness, setting its reference to null on dispose is not; it's just an optimization. Only, it makes the system slightly slower because it creates a small amount of extra work on disposal, with no benefit (since the resource in question isn't precious). It seems like you'd be better off just not overriding Dispose.

Looking at it more broadly, the design of Component is a big factor. Component implements IDisposable, yet its OK for the derived class SqlCommand to suppress the finalizer, which means Component doesn't have to be disposed; it's just an optimization. Is the optimization great enough to always require every derived class to have to be put in a using block, or worse, to need a manual call to Dispose if its lifetime exceeds a single method? This seems like a design hole that could have been corrected in the transition to .NET Core.

[Wraith2]

For SqlCommand specifically you're right that the tiny amount of work in Dispose isn't really needed but it isn't incorrect either. Setting it to zero will probably take a single intrusction which isn't needed and could be removed but i suspect that leaving that zero in place and breaking the link will make a JIT sweep easier since it won't need to follow the reference, so there's probably balance there.

Whether you should dispose of a command object is another question and depends on many factors. If you're specifically and only using SqlCommand then as you've said there is no requirement to do so. If you've programming to DbCommand where the implementation chould change then (as was pointed out in the SO thread) you absolutely should dispose of it because the iplementation may be written in a way that requires it. So my advice would be to be safe and always dispose disposable things unless you're absolutely sure they don't need it.

Changing this single line for performance reasons would require evidence that it is beneficial. I don't think it would need to be strong since the change itself is trivial but it would need to be quantifiable. Given how much happens when you do an sql query and the amount of GC activity that occurs i doubt you could be able to produce a benchmark that showed a realistic difference. It'd be nice to get to the point where such micro optimizations are useful but i think that may be a long way in the future.

[breyed]

The micro-benchmark isn't the core issue. I agree it would likely not show up. The point of not setting _cachedMetaData to null is to eliminate the Dispose method.

The benefit of eliminating Dispose is developer productivity. To do disposal correctly, every managed object that needs to be disposed incurs roughly the same data developer overhead as an unmanaged memory allocation in C++. If SqlCommand doesn't implement Disposable, or it can be documented that it callers can ignore the fact that it does, then callers have less bookkeeping to do when a SqlCommand outlives a method.

Even within a method, not having disposal allows more flexible use. For example, you can use property initializers. Using property initializers in a using statement statement isn't considered valid:

using (var command = new SqlCommand { Connection = c, CommandText = text, Parameters = { p1, p2 } }) { ... }

Such a construction is flagged by static code evaluation because one of the property setters could throw an exception causing the using block to not be entered and Dispose to not be called.

If SqlCommand will indeed have an implementation that requires Dispose, the productivity his is worth it. If not, the IDisposable overhead to the programmer is an instance of YAGNI.

A general way to look at it is that Disposable adds friction. It runs counter to the benefits of a managed environment, but sometimes it's necessary. In API design, it should be considered "high cost" in terms of developer productivity, and added only when the perf benefit outweighs the friction.

[Wraith2]

Removing the override of Dispose on SqlCommand will not make it non-disposable, dispose is implemented in the base class DbCommand to satisfy the IDbCommand interface. IDbCommand requiring IDisposable will not change because some derived implementations need the behaviour it requires for correctness.

Developers aren't required to use using blocks to handle disposable objects if they choose not to as long as they handle the lifetime appropriately it doesn't matter.

[breyed]

Agreed. The design question about the developer cost of SqlCommand.Dispose is really a holistic question about SqlCommand.Dispose, IDbCommand.Dispose, and Component..Dispose, due to the dependencies. It's unfortunate that there's no easy way for a developer to know whether his particular usage of SqlCommand requires lifetime tracking. Except when using an implementation that actually requires disposal, the effort to add it ends up having no practical benefit.

There doesn't seem to be much one can do about it. It's the price you pay for the flexibility of a single SqlCommand abstraction that covers differing implementations.

[vcsjones]

It's the price you pay

What exactly is the "price"? Clearing a field is not an expensive operation. Further, Dispose is a contract. SqlCommand may not do anything extremely useful in Dispose today, but it might one day.

[breyed]

The price is the developer time and code complexity. In the best case, it's having to add a using statement. In the worst case, it means that the class using SqlCommand has to now implement IDisposable and add disposal calls to Dispose and anywhere else it determines that it's done with the SqlCommand, then classes holding a reference to that class need to implement IDisposable, and so on.

Across a whole project, maybe it's one developer hour, times 100,000 projects, times an average $60 per developer hour. Completely wild guesses, but if they're about right, the price is $6,000,000.

[Clockwork-Muse]

...I'm not sure we could remove the interface anyways - that would be a pretty major breaking change, after all.

[breyed]

That was my point about "could have been corrected in the transition to .NET Core". Fortunately, there might still a good solution: Create a custom attribute (perhaps DisposeUnusedAttribute) that would inform developers, the IDE, and static code analysis that it's OK to skip the extra work of adding calls to Dispose. Other classes such a MemoryStream would benefit as well.

[Clockwork-Muse]

.... that's not really any different than adding/removing IDisposable. It's actually worse, because now you have something that says "yeah, ignore this contract that I claim to need", and if a type suddenly did have to dispose a resource (say, MemoryStream starts allocating a native array for really large array sizes or something), your consumers now need to dispose, but you previously informed them they didn't ...

A publicly visible attribute like that is likely to be permanent for the same general reason: breaking behavior change if removed.

[breyed]

IDisposable flows down the inheritance chain. Since Stream and Component implement it, all their derived classes are stuck with it, whether needed or not. Custom attributes don't have to be inherited.

The maybe-someday approach gets expensive. Taken to its logical conclusion, it would cause a lot of other classes to add just-in-case IDisposables. I'm really glad that TaskCompletionSource<TResult> doesn't implement IDisposable, even though I could see how you could implement it using unmanaged resources. The absence of the interface has saved me lots of book-keeping coding time. If someone comes up with a superior implementation but that requires IDisposable, we're not stuck; it just needs to be a new class. Not burdening the in-the-now for the sake of the maybe-someday is at the heart of the YAGNI principle.

[AfsanehR-zz]

@breyed @Wraith2 Thank you for your valuable inputs. Moving this to future as we are working on some higher priorities for now .

[divega]

As recently announced in the .NET Blog, focus on new SqlClient features an improvements is moving to the new Microsoft.Data.SqlClient package. For this reason, we are moving this issue to the new repo at https://github.com/dotnet/SqlClient. We will still use https://github.com/dotnet/corefx to track issues on other providers like System.Data.Odbc and System.Data.OleDB, and general ADO.NET and .NET data access issues.