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

Ensure lifetime of BuildCheckManager #10317

Closed
Tracked by #10316
JanKrivanek opened this issue Jul 1, 2024 · 2 comments · Fixed by #10649
Closed
Tracked by #10316

Ensure lifetime of BuildCheckManager #10317

JanKrivanek opened this issue Jul 1, 2024 · 2 comments · Fixed by #10649
Assignees
Labels
Cost:S Work that requires one engineer up to 1 week

Comments

@JanKrivanek
Copy link
Member

Context

BuildCheckManager is currently created once per node and survives for the entire lifetime of the node. This might be problematic as it contains state that should not be shared between builds - BuildChecks registration, ConfigurationModule, etc.

Possible solution

BuildRequestEngine contains InitializeForBuild and CleanupForBuild methods that anchor a single build. CleanupForBuild is already used to finalize the BuildCheckManager stats collection. We can add caches invalidation (if needed in the future - not now).

The InitializeForBuild should probably contain code ensuring that BuildCheckManager is recreated or reinitialized.

Alternative solution

#10009 will eventually contain solution for making BuildCheckManager instance based and probably owned by LoggingContext - so lifetime might not be of a concern anymore... but we are not there yet

BLOCKED BY #10145

@AR-May
Copy link
Member

AR-May commented Sep 11, 2024

This issue affects PropertiesUsageCheck behavior, as it is created on the out of process node. As node reuse is the default, if you run some solution build without /check and then run build with /check, PropertiesUsageCheck will not provide any results for the projects built in the reused out of process nodes.

@JanKrivanek
Copy link
Member Author

Note: MSBuild server migh need special handling - as it doesn't dispose singleton components:

msbuild/src/MSBuild/XMake.cs

Lines 1690 to 1693 in db5b3ef

if (!s_isServerNode)
{
BuildManager.DefaultBuildManager.Dispose();
}

@JanKrivanek JanKrivanek added the Cost:S Work that requires one engineer up to 1 week label Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cost:S Work that requires one engineer up to 1 week
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants