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.

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

Jump to bottom

Fix ArgumentNullException under a race condition in analyzer driver #65083

Merged
merged 9 commits into from
Nov 12, 2022
Merged

Fix ArgumentNullException under a race condition in analyzer driver #65083

merged 9 commits into from
Nov 12, 2022

Conversation

mavasani
Copy link
Contributor

@mavasani mavasani commented Oct 28, 2022 •
edited
Loading

Fixes #59988
Fixes AB#1654606

Do not mark declaration/symbol/event as fully processed when the state is still being processed by another thread. This can happen in rare cases when multiple threads are trying to analyze the same symbol/syntax/operation, such that one thread has finished all the analyzer callbacks, but before this thread attempts to mark the state as complete, second thread starts processing the same symbol/syntax/operation and sets the StateKind to 'InProcess' for the same state object. With this PR, we do not mark the state as complete for such cases and instead let subsequent thread attempt to mark completion.

@mavasani
Fix ArgumentNullException under a race condition in analyzer driver
ceed5d1 
Fixes dotnet#59988
Fixes [AB#1654606](https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1654606)

Do not free state if it is still being processed for analysis, i.e. StateKind is 'InProcess'. This can happen in rare cases when multiple threads are trying to analyze the same
symbol/syntax/operation, such that one thread has finished all the analyzer callbacks, but before this thread marks the state as complete and invokes this FreeState call,
second thread starts processing the same symbol/syntax/operation and sets the StateKind to 'InProcess' for the same state object. If we free the state here in the first thread,
then this can lead to data corruption/exceptions in the second thread. For example, see dotnet#59988.

Note that our current approach of not freeing the state for StateKind.InProcess leads to a leak, as we will never return this state object to the pool. However, this should
happen in extremely rare cases as described above, so it should be fine. In future, if we do see negative performance impact from this leak, we can do a more complex state tracking and
ensure that we free this state object and return it to the pool once the StateKind is reset to StateKind.ReadyToProcess state.
@mavasani mavasani requested a review from a team as a code owner October 28, 2022 05:09
@mavasani
Copy link
Contributor Author

@dotnet/roslyn-compiler for reviews

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 2)

mavasani
mavasani commented Nov 7, 2022
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisState.PerAnalyzerState.cs
@@ -375,9 +397,9 @@ public bool TryStartProcessingEvent(CompilationEvent compilationEvent, [NotNullW
return TryStartProcessingEntity(compilationEvent, _pendingEvents, _analyzerStateDataPool, out state);
}

public void MarkEventComplete(CompilationEvent compilationEvent)
public bool TryMarkEventComplete(CompilationEvent compilationEvent)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I converted all the methods that mark completion of events/symbols/declaration/syntax from void MarkXXXComplete(...) to bool TryMarkXXXComplete(...)

@mavasani
Refactor code to ensure proper state transitions happen during analys…
03b31e3 
…is - InProcess to ReadyToProcess to FullyProcessed
mavasani
mavasani commented Nov 8, 2022
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerExecutor.cs
AnalyzerStateData? analyzerState = null;

try
if (TryStartProcessingEvent(compilationEvent, analyzer, analysisScope, analysisState, out var analyzerState))
Copy link
Contributor Author

@mavasani mavasani Nov 8, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@AlekseyTs State machine transitions should now be very clear:

  1. We attempt to TryStartProcessingEvent to get hold of analyzer state, with StateKind.InProcess.
  2. If we succeed, then we attempt to execute relevant analyzer callbacks for the compilation event
  3. After executing callbacks, we transition the state to StateKind.ReadyToProcess state in the finally block surrounding execute actions method
  4. Finally, we attempt to mark completion of the event for the analyzer and try to transition to StateKind.FullyProcessed and return true only if we succeed.
  5. If we fail at Step 1, which happens when either another thread is executing steps 2-4 OR analyzer state has already completed and transitioned to FullyProcessed, we don't attempt to execute analysis but instead return IsEventComplete to handle the latter case.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Nov 9, 2022 •
edited
Loading 

                ClearCachedAnalysisDataIfAnalyzed(symbol, declarationIndex, analysisState);

Should we be taking this code path if we "failed" in the loop above?

In reply to: 1309059500

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs:2665 in 0f841ca. [](commit_id = 0f841ca, deletion_comment = False)

@AlekseyTs
Copy link
Contributor

Done with review pass (commit 7).

@mavasani mavasani mentioned this pull request Nov 10, 2022
Closed
mavasani
mavasani commented Nov 10, 2022
View reviewed changes
src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalysisState.AnalyzerStateData.cs
@@ -56,7 +57,7 @@ public void ResetToReadyState()

public virtual void Free()
{
this.StateKind = StateKind.ReadyToProcess;
Debug.Assert(StateKind == StateKind.ReadyToProcess);
Copy link
Contributor Author

@mavasani mavasani Nov 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Note that this will change to below after we implement #65330:

Debug.Assert(StateKind == StateKind.AttemptingToComplete);
this.StateKind = StateKind.ReadyToProcess;

@mavasani
Copy link
Contributor Author 

                ClearCachedAnalysisDataIfAnalyzed(symbol, declarationIndex, analysisState);

Should we be taking this code path if we "failed" in the loop above?

In reply to: 1309059500

Refers to: src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs:2665 in 0f841ca. [](commit_id = 0f841ca, deletion_comment = False)

Thanks, I missed this comment. Updated to also check success now.

AlekseyTs
AlekseyTs approved these changes Nov 10, 2022
View reviewed changes
Copy link
Contributor

@AlekseyTs AlekseyTs left a comment

Choose a reason for hiding this comment

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

LGTM (commit 9)

@mavasani
Copy link
Contributor Author

Thanks @AlekseyTs

@dotnet/roslyn-compiler for second review.

@mavasani
Copy link
Contributor Author

@dotnet/roslyn-compiler Can I please get a second review?
This change is a candidate for 17.4 servicing, and we want to give it enough bake time in 17.5 preview dogfood builds before taking it for 17.4 servicing. Thanks!

@cston
Copy link
Member

cston commented Nov 11, 2022

Do mark declaration/symbol/event as fully processed when the state is still being processed by another thread.

Should that be "Do not mark ... "?

@mavasani
Copy link
Contributor Author

Do mark declaration/symbol/event as fully processed when the state is still being processed by another thread.

Should that be "Do not mark ... "?

Yes, updated the PR description. Thanks.

@mavasani mavasani merged commit f814f00 into dotnet:main Nov 12, 2022
@mavasani mavasani deleted the AnalyzerStateRaceCondition branch November 12, 2022 09:41
@ghost ghost added this to the Next milestone Nov 12, 2022
@allisonchou allisonchou mentioned this pull request Nov 17, 2022
Closed
@mavasani mavasani restored the AnalyzerStateRaceCondition branch November 23, 2022 06:25
@mavasani mavasani mentioned this pull request Nov 23, 2022
Merged
@allisonchou allisonchou modified the milestones: Next, 17.5 P2 Nov 30, 2022
arunchndr added a commit that referenced this pull request Nov 30, 2022
@arunchndr
Merge pull request #65576 from mavasani/Port_AnalyzerStateRaceCondition
b77fbb2 
Port #65083 to release/dev17.4 branch
@Youssef1313 Youssef1313 mentioned this pull request Dec 7, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CyrusNajmabadi CyrusNajmabadi CyrusNajmabadi left review comments

@AlekseyTs AlekseyTs AlekseyTs approved these changes

@cston cston cston approved these changes

@333fred 333fred Awaiting requested review from 333fred

@RikkiGibson RikkiGibson Awaiting requested review from RikkiGibson

Assignees
No one assigned
Labels
Area-Analyzers Area-Compilers OnDeckForServicing
Projects
None yet
Milestone
17.5 P2
Development

Successfully merging this pull request may close these issues.

ArgumentNullException in Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteBlockActionsCore
6 participants
@mavasani @AlekseyTs @cston @CyrusNajmabadi @allisonchou @arunchndr