search

dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.96k stars 4.03k forks source link

IOperation SetParent Race Condition #35818

Open heejaechang opened 5 years ago

heejaechang commented 5 years ago 
[30176]    at Microsoft.CodeAnalysis.Operation.SetParentOperation(IOperation parent) in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\Operation.cs:line 142

[30176]    at Microsoft.CodeAnalysis.Operation.SetParentOperation[T](T operation, IOperation parent) in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\Operation.cs:line 163

[30176]    at Microsoft.CodeAnalysis.Operations.ArgumentOperation..ctor(IOperation value, ArgumentKind argumentKind, IParameterSymbol parameter, IConvertibleConversion inConversionOpt, IConvertibleConversion outConversionOpt, SemanticModel semanticModel, SyntaxNode syntax, Boolean isImplicit) in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\OperationNodes.cs:line 306

[30176]    at Microsoft.CodeAnalysis.Operations.CSharpOperationFactory.CreateArgumentOperation(ArgumentKind kind, IParameterSymbol parameter, BoundExpression expression) in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Operations\CSharpOperationFactory_Methods.cs:line 62

[30176]    at Microsoft.CodeAnalysis.CSharp.LocalRewriter.MakeArgumentsInEvaluationOrder(CSharpOperationFactory operationFactory, Binder binder, SyntaxNode syntax, ImmutableArray`1 arguments, Symbol methodOrIndexer, MethodSymbol optionalParametersMethod, Boolean expanded, ImmutableArray`1 argsToParamsOpt, Boolean invokedAsExtensionMethod) in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Lowering\LocalRewriter\LocalRewriter_Call.cs:line 599

[30176]    at Microsoft.CodeAnalysis.Operations.CSharpOperationFactory.DeriveArguments(BoundNode boundNode, Binder binder, Symbol methodOrIndexer, MethodSymbol optionalParametersMethod, ImmutableArray`1 boundArguments, ImmutableArray`1 argumentNamesOpt, ImmutableArray`1 argumentsToParametersOpt, ImmutableArray`1 argumentRefKindsOpt, ImmutableArray`1 parameters, Boolean expanded, SyntaxNode invocationSyntax, Boolean invokedAsExtensionMethod) in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Operations\CSharpOperationFactory_Methods.cs:line 337

[30176]    at Microsoft.CodeAnalysis.Operations.CSharpOperationFactory.DeriveArguments(BoundNode containingExpression) in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Operations\CSharpOperationFactory_Methods.cs:line 280

[30176]    at Microsoft.CodeAnalysis.Operations.CSharpLazyInvocationOperation.CreateArguments() in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Operations\CSharpOperationNodes.cs:line 799

[30176]    at Microsoft.CodeAnalysis.Operations.LazyInvocationOperation.get_Arguments() in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\OperationNodes.cs:line 3889

[30176]    at Microsoft.CodeAnalysis.Operations.BaseInvocationOperation.<get_Children>d__8.MoveNext() in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\OperationNodes.cs:line 3805

[30176]    at Microsoft.CodeAnalysis.Operations.OperationExtensions.<Descendants>d__3.MoveNext() in D:\dd\roslyn6\src\Compilers\Core\Portable\Operations\OperationExtensions.cs:line 85

[30176]    at System.Linq.Enumerable.FirstOrDefault[TSource](IEnumerable`1 source, Func`2 predicate)

[30176]    at Microsoft.CodeAnalysis.CSharp.MemberSemanticModel.GetOperationWorker(CSharpSyntaxNode node, CancellationToken cancellationToken) in D:\dd\roslyn6\src\Compilers\CSharp\Portable\Compilation\MemberSemanticModel.cs:line 1060

repro step. first clone this repo - https://github.com/nopSolutions/nopCommerce and enable full solution analysis in debug VS (assert is debug only assert)

....

another repro step. with Roslyn, enable VB full solution analysis, and open src\EditorFeatures\VisualBasicTest\CodeActions\MoveType\BasicMoveTypeTestsBase.vb with debug Roslyn bit.

jinujoseph commented 5 years ago

cc @mavasani

333fred commented 5 years ago

This appears to be a threading issue: http://source.roslyn.io/#Microsoft.CodeAnalysis.CSharp/Operations/CSharpOperationFactory_Methods.cs,61. Multiple threads could be attempting to get the arguments of an invocation at the same time, and one of these threads could hit the cache if the timing is correct. The operation returned from the cache already has a parent operation, a different ArgumentOperation, so the debug assert is hit. Most of the time, we should end up in an OK state afterwards because the Interlocked.CompareExchange when creating the arguments will not set the new arguments, as arguments were already set, but there is a chance that we could end up with split trees if the thread timing is just right. The solution is to ensure that any non-lazy operation creation does not hit the cache in the factories.

333fred commented 5 years ago

The assert that was revealing this race is being disabled for now in https://github.com/dotnet/roslyn/pull/39018, to allow tests to pass. Our plan for fixing the race is to make IOperation non-lazy, which will have some additional benefits:

  1. IOperations are primarily used by analyzers that register for callbacks. Because of this, we have to create all IOperations anyway, in order to ensure that we actually called back all registered analyzers correctly. This laziness from IOperation just buys us extra overhead.
  2. Fixing the races while being lazy would either require very coarse-grained locks, heavily increasing contention, or trying to be really clever with multi-threaded mutation, which is something that roslyn's current design tries to avoid as much as possible.