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

Fix cache exception handling #6110

Merged
merged 1 commit into from Mar 3, 2021

Conversation

cdmihai
Copy link
Contributor

@cdmihai cdmihai commented Feb 3, 2021

Fixes two issues when the plugin throws an exception in Plugin.EndBuildAsync during graph builds:

Copy link
Member

@Forgind Forgind left a comment

Choose a reason for hiding this comment

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

Just a note that moving or refactoring a lot of code makes it much harder to review PRs, even when it's in a separate commit.


while (blockedNodes.Count > 0 || buildingNodes.Count > 0)
Dictionary<ProjectGraphNode, BuildResult> resultsPerNode;
using (cacheServiceTask.Result)
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain why this changes the ordering versus when the graph build returns?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

using variables get disposed at the end of the block they're in. In this case the end of the block is after the last line in the block whish is the call to ReportResultsToSubmission, but the cache needs to be disposed (in order to surface any issues in EndBuildAsync) before the call to ReportResultsToSubmission.

@cdmihai cdmihai force-pushed the fixCacheExceptionHandling branch 3 times, most recently from 7b9afd9 to 9e5cfc3 Compare February 4, 2021 00:23
Copy link
Contributor

@rokonec rokonec left a comment

Choose a reason for hiding this comment

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

Looks OK.

@rokonec rokonec added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Feb 26, 2021
@rokonec rokonec merged commit 746f19b into dotnet:master Mar 3, 2021
@cdmihai cdmihai deleted the fixCacheExceptionHandling branch March 3, 2021 18:11
@cdmihai cdmihai restored the fixCacheExceptionHandling branch April 14, 2021 00:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants