Dispose DbCommand in DbMetaDataFactory.ExecuteCommand #103593
Conversation
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
| @@ -122,7 +122,7 @@ private DataTable ExecuteCommand(DataRow requestedCollectionRow, string?[]? rest | |||
| throw ADP.TooManyRestrictions(collectionName); | |||
| } | |||
|
|
|||
| command = connection.CreateCommand(); | |||
There was a problem hiding this comment.
Move the declaration of command together with assignment:
using DbCommand command = connection.CreateCommand();
huoyaoyuan
added
area-System.Data
and removed
needs-area-label
|
Tagging subscribers to this area: @roji, @ajcvickers |
A static analysis tool has flagged that DbCommand is an IDisposable, so it should be Disposed() to free up resources.
omajid
force-pushed
the
dispose-command-dbmetadatafactory
branch
from
8e6b2a2 to
f7ec31e
Compare
|
We discussed this in the data team triage, and we don't think we should do this. DbCommand indeed implements IDisposable, since for legacy reasons it extends Component. Now, in System.Data there is a long tradition of DbCommand not actually requiring disposal; EF6 did not dispose DbCommands (though EF Core does, IIRC), and ADO.NET providers generally take to not require DbCommand to be disposed (both the Npgsql and Firebird had to be modified to not leak resources via undisposed commands). Now, that in itself isn't a reason to not dispose commands, and both users and ORMs do so. However, this PR specifically changes a piece of very old code within System.Data, which hasn't been touched in a long time, and the bar for touching it is generally very high. Because of this, we'd rather prioritize stability here and keep the code as-is, unless a demonstrated problem is reported on this. So I'm going to go ahead and close it for now. |
|
Thanks for the explanation. Your decision makes sense to me. |
A static analysis tool has flagged that DbCommand is an IDisposable, so it should be Disposed() to free up resources.