Avoid false assert in AssertMemberExposure due to a race.#53044
Avoid false assert in AssertMemberExposure due to a race.#53044AlekseyTs merged 2 commits intodotnet:mainfrom
Conversation
|
|
||
| RoslynDebug.AssertOrFailFast(false, "Premature symbol exposure."); | ||
|
|
||
| static bool isMemberInCompleteMemeberList(MembersAndInitializers? membersAndInitializers, Symbol member) |
There was a problem hiding this comment.
| static bool isMemberInCompleteMemeberList(MembersAndInitializers? membersAndInitializers, Symbol member) | |
| static bool isMemberInCompleteMemberList(MembersAndInitializers? membersAndInitializers, Symbol member) |
333fred
left a comment
There was a problem hiding this comment.
LGTM (commit 1) other than the typo.
|
@dotnet/roslyn-compiler For a second review. |
| else | ||
| { | ||
| // It looks like there was a race and we need to check _lazyMembersAndInitializers again | ||
| membersAndInitializers = Volatile.Read(ref _lazyMembersAndInitializers); |
There was a problem hiding this comment.
In order for this to be safe I believe we need to switch the following assignment to use a Volatile.Write call
var alreadyKnown = Interlocked.CompareExchange(ref _lazyMembersAndInitializers, membersAndInitializers, null);
if (alreadyKnown != null)
{
diagnostics.Free();
return alreadyKnown;
}
AddDeclarationDiagnostics(diagnostics);
diagnostics.Free();
// SUGGESTION: make this a Volatile.Write
_lazyDeclaredMembersAndInitializers = null;
return membersAndInitializers!;The CompareExchange(ref _lazyMembersAndInitializes, ...) uses a fully memory barrier but I believe that occurs before the operation not after. That means all of the writes after the operation can be viewed by other thread in any order. Hence it's possible for a different thread to observe _lazyDeclaredMembersAndInitializers = null before it sees the write to _lazyMembersAndInitiaizers. That is true even though you use Volatile.Read because that only affects reordering on the current thread.
Rule of thumb is that if you use Volatile.Read on one thread they need to be paired with Volatile.Write or Intelocked.Exchange on the other thread. Else you can get into ordering issues.
I'm not 100% confident in my assessment here, memory ordering is tricky. So possible someone comes back and corrects me here but would feel better with this change.
There was a problem hiding this comment.
Followed up with runtime team. My reasoning is incorrect. The CompareExchange does a fence before / after hence the writes are ordered.
…into bind-strings * upstream/features/interpolated-string: (912 commits) Update version for 16.11 Avoid false assert in AssertMemberExposure due to a race. (dotnet#53044) Fix caret placement for argument completion snippets Explicitly define no PR triggers as they are on 'by default' Remove eng/docker/mono.sh script (dotnet#51833) Run LSP integration on CI builds only until they're stable Implement the right interface Disable CompileTimeSolutionProvider Avoid negative logic when checking whether a eol is present Update src/Analyzers/CSharp/CodeFixes/UseSimpleUsingStatement/UseSimpleUsingStatementCodeFixProvider.cs Apply suggestions Adapt codestyle Prepend first statement of using block with a newline, if opening bracket is on same line as using statement Fix EnC capabilties check for runtimes that aren't returning capabilities yet (dotnet#53000) Add ConfigureAwait calls to cocoa code. Remove CustomTags from CodeAction constructors Make CodeAction.CustomTags internal Update "contributing" doc with pointers on IOperation test hook (dotnet#52978) Cocoa part Switch to a simpler cancellation ownership model for the FAR window. (Cocoa still to come) ...
Fixes #52368.