Fixes CA2213 issues on NuGet client repo by dominoFire · Pull Request #4812 · NuGet/NuGet.Client

dominoFire · 2022-09-20T01:23:14Z

Bug

Fixes: NuGet/Home#12116

Regression? Last working version: N/A

Description

Fixes CA2213 rule by calling Dispose() on internal HTTP client on HttpSourceAuthenticationHandler class
Fixes CA2213 rule in test code
Added an inline suppression in test code, since Analyzer thinks the rule is not solved.
Added unit tests for HttpSourceAuthenticationHandler.Dispose()
Throw ObjectDisposedException when calling SendAsync() on HttpSourceAuthenticationHandler disposed object
Added backslash dir separator in project files to fix issues in my devbox when using a NuGetPackageRoot without trailing backslash char.

PR Checklist

PR has a meaningful title
PR has a linked issue.
Described changes
Tests
- Automated tests added: Added 3 tests for product code
- OR
- Test exception
- OR
- N/A
Documentation
- Documentation PR or issue filled
- OR
- N/A: No product change

donnie-msft

Thanks for jumping on this! I just have some questions about testing strategy...

src/NuGet.Clients/NuGet.PackageManagement.UI/NuGet.PackageManagement.UI.csproj

test/NuGet.Core.Tests/NuGet.Protocol.Tests/HttpSource/HttpSourceAuthenticationHandlerTests.cs

nkolev92 · 2022-09-20T18:52:14Z

src/NuGet.Core/NuGet.Protocol/HttpSource/HttpSourceAuthenticationHandler.cs

            HttpHandlerResourceV3.CredentialsSuccessfullyUsed?.Invoke(uri, credentials);
        }
+
+        protected override void Dispose(bool disposing)


Do we ever call dispose on the handler?

The thing about the dispose pattern is that it needs to be implement all the way up the stack. Are we doing that?

It seems no.

HttpSourceAuthenticationHandler objects are created inside HttpHandlerResourceV3Provider.TryCreate() method. Objects returned by this method are INuGetResource-based. This interface does not implement any IDisposable-based interfaces.

In short words: we don't call dispose on this class in NuGet.Client source code. We are now just being good citizens by implementing Dispose.

How about we just accept this dispose changes as it's? But create/link another follows up issue for implementing dispose pattern all the way up the stack, fixing that part shouldn't be high priority for now.
Or just suppress with reason why it doesn't add any value by doing so.

I'm looking forward to merging this PR. Added follow up issue to use IDisposable pattern in NuGet protocol code. NuGet/Home#12176

Should we call GC.SuppressFinalize(this); in dispose? We're doing it for another dispose.

NuGet.Client/src/NuGet.Clients/NuGet.PackageManagement.VisualStudio/Services/NuGetSourcesService.cs

Line 89 in 10ce64e

GC.SuppressFinalize(this);

Not here, that should be called in Dispose() method (without arguments) of base class

Not here, that should be called in Dispose() method (without arguments) of base class

Sure, thank you for clarifying.

test/TestUtilities/Test.Utility/Protocol/TestContent.cs

dominoFire · 2022-10-05T21:14:22Z

Team review: Add tests that dispose multiple times to verify nothing else throws exceptions.

ghost · 2022-10-12T22:01:29Z

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 7 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

erdembayar

I believe we address 1 problem at the time, so this is good enough for merging.

Fixes NuGet/Home#12116 Fixes CA2213 in HttpSourceAuthenticationHandler and in test code. Left 1 CA2213 suppression due to a Roslyn analyzer bug linked in the suppression code.

dominoFire marked this pull request as ready for review September 20, 2022 04:06

dominoFire requested a review from a team as a code owner September 20, 2022 04:06

donnie-msft reviewed Sep 20, 2022

View reviewed changes

nkolev92 reviewed Sep 20, 2022

View reviewed changes

dominoFire force-pushed the dev-dominofire-fixca2213 branch from abdb7f8 to f29615d Compare September 21, 2022 20:11

dominoFire mentioned this pull request Sep 22, 2022

CA2213: False positive on disposable fields dotnet/roslyn-analyzers#6172

Closed

dominoFire requested review from a team, donnie-msft, kartheekp-ms and nkolev92 September 22, 2022 20:35

zivkan previously approved these changes Sep 26, 2022

View reviewed changes

test/TestUtilities/Test.Utility/Protocol/TestContent.cs Outdated Show resolved Hide resolved

dominoFire dismissed zivkan’s stale review via 42779e2 September 26, 2022 22:04

dominoFire force-pushed the dev-dominofire-fixca2213 branch from 9daaf35 to 42779e2 Compare September 26, 2022 22:04

dominoFire requested review from a team and zivkan September 26, 2022 22:06

dominoFire force-pushed the dev-dominofire-fixca2213 branch from 42779e2 to 45bd994 Compare September 29, 2022 19:46

ghost added the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 12, 2022

Fernando Aguilar added 11 commits October 18, 2022 00:31

Attempt to fix CA2213

d493072

Fix attempts

64765a2

Tests: Added 1 test and fixed CA2213 on flagged test classes

d673a13

Tests: code cleaning

296494f

Tests: Disabled CA2213 rule on test content

df1e992

Removed N.PM.UI warning for now. Save it for later in shopping cart :)

f5a15f8

Removed test

2987c96

Added roslyn issue

4752b7f

Removed TODOs

22faa5e

Fixed path to work with NuGetPackageRoot without dir separator

265bcb2

Throw when disposed on SendAsync()

181dc73

Added tests

714a6b6

dominoFire force-pushed the dev-dominofire-fixca2213 branch from 3467f70 to 714a6b6 Compare October 18, 2022 20:16

ghost removed the Status:No recent activity PRs that have not had any recent activity and will be closed if the label is not removed label Oct 18, 2022

Fernando Aguilar added 4 commits October 18, 2022 17:25

Tests: separating work and dispose steps

41be37e

Test: Fix test

e00b390

Updated issue

eb88841

Test: Fix other test

ce6fedc

dominoFire requested a review from erdembayar October 20, 2022 08:25

erdembayar approved these changes Oct 20, 2022

View reviewed changes

dominoFire merged commit d1620d3 into dev Oct 21, 2022

dominoFire deleted the dev-dominofire-fixca2213 branch October 21, 2022 00:47

This was referenced Jan 24, 2023

Improve VSfeedback data 6.2 #5031

Merged

Improve VSfeedback data 6.0 #5033

Merged

Conversation

dominoFire commented Sep 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Bug

Description

PR Checklist

Uh oh!

donnie-msft left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nkolev92 Sep 20, 2022

Choose a reason for hiding this comment

Uh oh!

dominoFire Sep 21, 2022

Choose a reason for hiding this comment

Uh oh!

dominoFire Sep 22, 2022

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dominoFire Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

dominoFire Oct 20, 2022

Choose a reason for hiding this comment

Uh oh!

erdembayar Oct 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dominoFire commented Oct 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 12, 2022

Uh oh!

erdembayar left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dominoFire commented Sep 20, 2022 •

edited

Loading

erdembayar Oct 19, 2022 •

edited

Loading

erdembayar Oct 20, 2022 •

edited

Loading

dominoFire commented Oct 5, 2022 •

edited

Loading