Skip to content

Don't cache known-broken compositions #80021

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Aug 22, 2025
Merged

Don't cache known-broken compositions #80021

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/Workspaces/Remote/Core/ExportProviderBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,12 +87,13 @@ private async Task<IExportProviderFactory> GetCompositionConfigurationAsync(Canc
// Assemble the parts into a valid graph.
var config = CompositionConfiguration.Create(catalog);

// Verify we only have expected errors.

ReportCompositionErrors(config, catalog);

// Try to cache the composition.
_ = WriteCompositionCacheAsync(compositionCacheFile, config, cancellationToken).ReportNonFatalErrorAsync();
// Check if we have errors, and report them accordingly.
if (!CheckForAndReportCompositionErrors(config, catalog))
{
// There weren't any errors in the composition, so let's cache it. If there were errors, those errors might have been temporary and we don't want
// to end up in a permanently broken case.
_ = WriteCompositionCacheAsync(compositionCacheFile, config, cancellationToken).ReportNonFatalErrorAsync();
}

// Prepare an ExportProvider factory based on this graph.
return config.CreateExportProviderFactory();
Expand Down Expand Up @@ -182,10 +183,14 @@ protected virtual void PerformCacheDirectoryCleanup(DirectoryInfo directoryInfo,

protected abstract bool ContainsUnexpectedErrors(IEnumerable<string> erroredParts);

private void ReportCompositionErrors(CompositionConfiguration configuration, ComposableCatalog catalog)
/// <returns>True if there was an unexpected composition error, false otherwise.</returns>
private bool CheckForAndReportCompositionErrors(CompositionConfiguration configuration, ComposableCatalog catalog)
{
var hasErrors = false;

foreach (var exception in catalog.DiscoveredParts.DiscoveryErrors)
{
hasErrors = true;
LogError($"Encountered exception in the MEF composition: {exception.Message}");
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi Aug 22, 2025

Choose a reason for hiding this comment

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

Will this come back to us as telemetry?

Copy link
Member Author

@jasonmalinowski jasonmalinowski Aug 22, 2025

Choose a reason for hiding this comment

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

And yes, at least indirectly: this goes through the gold bar reporting, which then logs telemetry for this message. I can see telemetry for myself or @phil-allen-msft hitting this exact line.

}

Expand All @@ -194,6 +199,8 @@ private void ReportCompositionErrors(CompositionConfiguration configuration, Com

if (ContainsUnexpectedErrors(erroredParts))
{
hasErrors = true;

try
{
configuration.ThrowOnErrors();
Expand All @@ -204,5 +211,7 @@ private void ReportCompositionErrors(CompositionConfiguration configuration, Com
LogError($"Encountered errors in the MEF composition: {ex.Message}{Environment.NewLine}{ex.ErrorsAsString}");
}
}

return hasErrors;
}
}
Loading