Chain capabilities snapshot to nominate restore and dependency tree dataflows#3167
Merged
natidea merged 2 commits intodev15.6-preview3from Jan 23, 2018
Merged
Chain capabilities snapshot to nominate restore and dependency tree dataflows#3167natidea merged 2 commits intodev15.6-preview3from
natidea merged 2 commits intodev15.6-preview3from
Conversation
This is to ensure accesses to OrderPrecedenceImportCollection (i.e. used to import rule handlers and tree view providers) occur in the right capabilities context. Occurrences in the wrong context led to a race condition in OrderPrecedenceImportCollection.GetFilteredSnapshot() which resulted in these collections being empty, breaking the dependencies tree.
Pilchie
approved these changes
Jan 19, 2018
natidea
changed the title
Contributor
Author
|
/cc @lifengl |
Contributor
Author
|
/cc @dotnet/project-system |
Pilchie
approved these changes
Jan 23, 2018
|
|
||
| await HandleAsync(e, handlerType).ConfigureAwait(false); | ||
|
|
||
| using (ProjectCapabilitiesContext.CreateIsolatedContext(configuredProject, e.Value.Item3)) |
Member
There was a problem hiding this comment.
Is it safe to await inside this using?
Member
|
I'm okay with moving forward with this fix, but I'd really like to know what has caused all of these races to start appearing now? AFAIK, much of this hookup hasn't changed in a very long time. |
lifengl
approved these changes
Jan 23, 2018
Contributor
lifengl
left a comment
lifengl
left a comment
There was a problem hiding this comment.
the change looks good to me
Contributor
Author
|
Merging optimistically to catch insertion train. |
This was referenced Jan 23, 2018
Member
Member
|
Also, what's the rules around when should we use a isolated capability snapshot? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A race condition in CPS is resulting from data flow action blocks being executed in the wrong capabilities context. This race condition may have been around for a while but is occurring much more frequently. CPS expects us to explicitly chain dataflows to the right capability context, so we are doing so in a number of cases where the symptoms of this race condition have become more pronounced. This PR fixes two cases:
OrderPrecedenceImportCollection(used to import rule handlers and tree view providers) to return an empty collection fromOrderPrecedenceImportCollection.GetFilteredSnapshot(), breaking the dependencies tree.Customer scenario
Customers opening .NET Core projects sometimes see spurious warnings in the dependencies tree, or an empty dependencies tree node, and sometimes restore fails to run.
Bugs this fixes:
Fixes #3111
Fixes 553715
Workarounds, if any
Building the solution generally resolves case 1
Close and reopen solution sometimes resolves case 2
Risk
Should be low as the rest of the dataflow is unchanged
Performance impact
Low. Only change is to creating Isolated capabilities context, which is fast and necessary in any case
Is this a regression from a previous update?
Appears to have gotten worse in the latest bits, but its not clear why
Root cause analysis:
Pending: We are still trying to figure out the change that makes the race condition more frequent.
How was the bug found?
Numerous internal reports
Notes
I'm creating a signed build and VAL build to test this further.