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

Add dispose rule and handle it in solution (production part) #9983

Conversation

YuliiaKovalova
Copy link
Member

@YuliiaKovalova YuliiaKovalova commented Apr 9, 2024

Context

The absence of this rule cause a memory leak during execution.

Changes Made

Enable CA2000 rule for production code and temporary make exception for test projects.

@YuliiaKovalova YuliiaKovalova changed the title add dispose part 1 add dispose rule and handle it in solution Apr 10, 2024
src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved
src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs Outdated Show resolved Hide resolved
src/Deprecated/Engine/Engine/IntrinsicFunctions.cs Outdated Show resolved Hide resolved
src/Framework/NativeMethods.cs Outdated Show resolved Hide resolved
src/Tasks/ManifestUtil/XmlUtil.cs Outdated Show resolved Hide resolved
src/Tasks/DownloadFile.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/enable_disposable_rule_check branch from fd0332a to aa28c4f Compare April 11, 2024 08:03
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/enable_disposable_rule_check branch from aa28c4f to 6b56301 Compare April 11, 2024 08:05
@YuliiaKovalova YuliiaKovalova changed the title add dispose rule and handle it in solution Add dispose rule and handle it in solution (production part) Apr 11, 2024
@YuliiaKovalova YuliiaKovalova marked this pull request as ready for review April 11, 2024 12:09
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

I have partially reviewed the PR, leaving comments inline.

src/Build/BackEnd/BuildManager/BuildManager.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Node/OutOfProcServerNode.cs Outdated Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
src/Tasks/ManifestUtil/Util.cs Outdated Show resolved Hide resolved
src/Tasks/ManifestUtil/SecurityUtil.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/enable_disposable_rule_check branch from 8783283 to 6ef1733 Compare April 11, 2024 19:57
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Leaving some more inline comments (28/50 files reviewed).

src/Build/BackEnd/Node/OutOfProcServerNode.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Node/OutOfProcServerNode.cs Show resolved Hide resolved
src/Build/Evaluation/IntrinsicFunctions.cs Outdated Show resolved Hide resolved
src/Deprecated/Engine/Engine/IntrinsicFunctions.cs Outdated Show resolved Hide resolved
src/Framework/NativeMethods.cs Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
src/Tasks/Unzip.cs Outdated Show resolved Hide resolved
src/Tasks/Unzip.cs Outdated Show resolved Hide resolved
src/Tasks/ManifestUtil/Util.cs Outdated Show resolved Hide resolved
src/Tasks/GenerateResource.cs Outdated Show resolved Hide resolved
src/Tasks/GenerateResource.cs Outdated Show resolved Hide resolved
src/Tasks/GenerateResource.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Shared/TargetResult.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Shared/BuildRequestConfiguration.cs Outdated Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova force-pushed the dev/ykovalova/enable_disposable_rule_check branch from 96b0b7c to c8fee7e Compare April 16, 2024 10:10
Copy link
Member

@ladipro ladipro left a comment

Choose a reason for hiding this comment

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

Leaving a few more comments inline.

src/Tasks/ManifestUtil/SecurityUtil.cs Outdated Show resolved Hide resolved
src/MSBuild/XMake.cs Outdated Show resolved Hide resolved
src/Framework/NativeMethods.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Shared/TargetResult.cs Outdated Show resolved Hide resolved
src/Build/BackEnd/Shared/BuildRequestConfiguration.cs Outdated Show resolved Hide resolved
src/Tasks/WriteCodeFragment.cs Outdated Show resolved Hide resolved
src/Tasks/DownloadFile.cs Outdated Show resolved Hide resolved
src/Tasks/ManifestUtil/ManifestWriter.cs Outdated Show resolved Hide resolved
src/Framework/NativeMethods.cs Outdated Show resolved Hide resolved
src/Tasks/Unzip.cs Show resolved Hide resolved
@YuliiaKovalova YuliiaKovalova requested a review from a team as a code owner April 19, 2024 08:13
@YuliiaKovalova YuliiaKovalova enabled auto-merge (squash) April 19, 2024 15:06
@YuliiaKovalova YuliiaKovalova merged commit d610389 into dotnet:main Apr 19, 2024
10 checks passed
rainersigwald pushed a commit that referenced this pull request Apr 30, 2024
… available in the ClickOnce's bootstrapper packages folder (#10093)

#9983 introduced a regression in ClickOnce's bootstrapper packages builder code by disposing an XmlReader object while it's still in use.

Fix the bootstrapper builder code that validates against a schema file to use the XmlReaderSettings instead of the deprecated XmlValidatingReader class.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants