Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dispose DbCommand in DbMetaDataFactory.ExecuteCommand #103593

Closed
wants to merge 1 commit into from

Conversation

omajid
Copy link
Member

@omajid omajid commented Jun 17, 2024

A static analysis tool has flagged that DbCommand is an IDisposable, so it should be Disposed() to free up resources.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jun 17, 2024
@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jun 17, 2024
@@ -122,7 +122,7 @@ private DataTable ExecuteCommand(DataRow requestedCollectionRow, string?[]? rest
throw ADP.TooManyRestrictions(collectionName);
}

command = connection.CreateCommand();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move the declaration of command together with assignment:

using DbCommand command = connection.CreateCommand();

@huoyaoyuan huoyaoyuan added area-System.Data and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jun 19, 2024
Copy link
Contributor

Tagging subscribers to this area: @roji, @ajcvickers
See info in area-owners.md if you want to be subscribed.

A static analysis tool has flagged that DbCommand is an IDisposable, so
it should be Disposed() to free up resources.
@roji
Copy link
Member

roji commented Aug 29, 2024

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.

@roji roji closed this Aug 29, 2024
@omajid
Copy link
Member Author

omajid commented Aug 29, 2024

Thanks for the explanation. Your decision makes sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Data community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants