Fix race conditions in attribute asserts#52486
Conversation
There are asserts in our `CustomAttributeBag` to verify we maintain the invariant that early decoding occurs before the full decoding. The asserts though done in the getters and take the following form: ```cs bool earlyComplete = IsPartComplete(CustomAttributeBagCompletionPart.EarlyDecodedWellKnownAttributeData); // If late attributes are complete, early attributes must also be complete Debug.Assert(!IsPartComplete(CustomAttributeBagCompletionPart.DecodedWellKnownAttributeData) || earlyComplete); return earlyComplete; ``` This pattern is subject to race conditions. Consider the case where the `bool earlyComplete` statement runs to completion and returns `false`. Then another thread swaps in and completes both early and full decoding. At that point the original thread resumes and the `Debug.Assert` will fail yet no invariant has been violated. Moved the `Debug.Assert` into the setters where we can reliably test the state invariants. closes dotnet#52372 related to 52368
|
@dotnet/roslyn-compiler PTAL |
|
@jaredpar It looks like bootstrap build is broken. Could it be related to the changes around the asserts? |
|
The tests were failing due to the asserts I added. I had to remove one because it's a fundamentally racy scenario. It's not really possible to assert full decoding hasn't completed before noting early decoding has completed. Another thread can always interrupt and do an early + full decode. Hence I had to remove that. I left the assert that we verify that early decoding is done before full decoding. That is safe. The bootstrap seems to be failing for the existing issue around the VB compiler crashing that I've been looking into. It's been tough to track down because the compiler is apparently exiting with no information. Been working with Sam on how to best track that down. |
| // Early decode must complete before full decode | ||
| Debug.Assert(!IsPartComplete(CustomAttributeBagCompletionPart.DecodedWellKnownAttributeData)); | ||
| var setOnOurThread = Interlocked.CompareExchange(ref _earlyDecodedWellKnownAttributeData, data, null) == null; | ||
| NotePartComplete(CustomAttributeBagCompletionPart.EarlyDecodedWellKnownAttributeData); |
There was a problem hiding this comment.
Debug.Assert(!IsPartComplete(CustomAttributeBagCompletionPart.DecodedWellKnownAttributeData)); [](start = 12, length = 94)
Perhaps instead of removing the assert it could be changed to:
Debug.Assert(!IsPartComplete(CustomAttributeBagCompletionPart.DecodedWellKnownAttributeData) || IsPartComplete(CustomAttributeBagCompletionPart.EarlyDecodedWellKnownAttributeData));
``` #Closed
There was a problem hiding this comment.
Good suggestion. I think I've convinced myself that this is safe 😄
I was concerned about potential read reordering issues but these are just loading the same block of memory and the state changes are atomic on that same piece of memory. Hence this should be robust to that type of problem.
| # Rebuilds with compilation errors | ||
| # Rebuilds with missing references | ||
| # Rebuilds with other issues | ||
| " --exclude net472\Microsoft.CodeAnalysis.EditorFeatures2.UnitTests.dll" + |
There was a problem hiding this comment.
Was this change necessitated by other changes in the PR?
There was a problem hiding this comment.
No, I think there is a flaky issue there. I saw it pop up on a few other PRs. I'm looking at the diff locally right now to seew hat's going on.
There are asserts in our
CustomAttributeBagto verify we maintain theinvariant that early decoding occurs before the full decoding. The
asserts though done in the getters and take the following form:
This pattern is subject to race conditions. Consider the case where the
bool earlyCompletestatement runs to completion and returnsfalse. Thenanother thread swaps in and completes both early and full decoding. At
that point the original thread resumes and the
Debug.Assertwill failyet no invariant has been violated.
Moved the
Debug.Assertinto the setters where we can reliably test thestate invariants.
closes #52372
related to #52368