Skip to content

Conversation

@michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Mar 7, 2025

In most cases, there is no need to dispose tasks (it is not an anti pattern, not to do so).
In this PR we no longer flag undisposed tasks in the cs/local-not-disposed query.

DCA Looks good.
(cc @rvermeulen)

@github-actions github-actions bot added the C# label Mar 7, 2025
@michaelnebel michaelnebel changed the title C#: cs/local-not-disposed on Task C#: Exclude Task from cs/local-not-disposed. Mar 10, 2025
@michaelnebel michaelnebel force-pushed the csharp/localnotdisposed branch from 16391a9 to 13226ed Compare March 10, 2025 09:54
@michaelnebel michaelnebel marked this pull request as ready for review March 10, 2025 10:40
Copilot AI review requested due to automatic review settings March 10, 2025 10:40
@michaelnebel michaelnebel requested a review from a team as a code owner March 10, 2025 10:40
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

PR Overview

This PR updates the local disposal query for C# to no longer flag un-disposed tasks, in line with best practices noted in the referenced blog post.

  • Adds System.Threading.Tasks namespace and an example using Task to verify that un-disposed tasks are acceptable in this context.
  • Updates change notes to document that un-disposed tasks are now allowed.

Reviewed Changes

File Description
csharp/ql/test/query-tests/API Abuse/NoDisposeCallOnLocalIDisposable/NoDisposeCallOnLocalIDisposable.cs Adds a test case demonstrating that Tasks are not flagged for disposal.
csharp/ql/src/change-notes/2025-03-10-task-not-disposed.md Adds change note documentation for the new behavior.

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Tip: Copilot only keeps its highest confidence comments to reduce noise and keep you focused. Learn more

…osable/NoDisposeCallOnLocalIDisposable.cs

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Contributor

@tamasvajk tamasvajk left a comment

Choose a reason for hiding this comment

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

LGTM. I think we might want to do the same exclusion for System.Net.Http.HttpClient.

@michaelnebel
Copy link
Contributor Author

michaelnebel commented Mar 12, 2025

LGTM. I think we might want to do the same exclusion for System.Net.Http.HttpClient.

Could you elaborate a bit on why?
AFAICT there is recommendation to only have one such (static) object and in this case, we don't need to expose it, but this use-case will not be flagged by our query as it is. On the other hand if a local HttpClient is created, then it should still be disposed?

@michaelnebel michaelnebel merged commit 4a3e463 into github:main Mar 12, 2025
20 checks passed
@michaelnebel michaelnebel deleted the csharp/localnotdisposed branch March 12, 2025 08:17
@tamasvajk
Copy link
Contributor

Could you elaborate a bit on why? AFAICT there is recommendation to only have one such (static) object and in this case, we don't need to expose it, but this use-case will not be flagged by our query as it is. On the other hand if a local HttpClient is created, then it should still be disposed?

We can discuss this on Slack, but I'm under the impression that HttpClient instances should be created through an IHttpClientFactory. The clients themselves can be locals, and they don't need to be disposed, because the factory is managing the lifetimes of the underlying HttpMessageHandlers.

Ref: https://learn.microsoft.com/en-us/aspnet/core/fundamentals/http-requests?view=aspnetcore-9.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants