Roslyn: Compiler fails while compiling code with excessive usage of expressions

[renezweifel]

Version Used: Commit from master 28688494 (appears in VisualStudio 2017 [15.7.4] and previous versions)

Steps to Reproduce:

1. Copy attached code from file RoslynIssue.txt into file CommandLineTests.cs in project CSharpCommandLineTest
2. Run (or Debug) new Test TonsOfExpressions
3. Test fails because of exception while compiling

Expected Behavior:
Code is compiling

Actual Behavior:
We have a big solution with excessive usage of Expressions. Project fails to compile with csc failure.

Assumption:
Call analyzedArguments.Free() in class Microsoft.CodeAnalysis.CSharp.Conversions on line 76 is wrong. But as we are not very familiar with this code we prefer to report as issue instead of fixing and create a pullrequest....

Relates to #27523

[jcouv]

Thanks for the precise repro.
Just to confirm when I run this test, I get a System.NullReferenceException. From what I can tell, we're iterating through a builder with Length 0, and getting one item which is null.

  HResult=0x80004003
  Message=Object reference not set to an instance of an object.
  Source=Microsoft.CodeAnalysis.CSharp
  StackTrace:
   at Microsoft.CodeAnalysis.CSharp.OverloadResolutionResult`1.<>c__18`1.<ReportDiagnostics>b__18_0(BoundExpression a) in D:\repos\roslyn\src\Compilers\CSharp\Portable\Binder\Semantics\OverloadResolution\OverloadResolutionResult.cs:line 204

[renezweifel]

Exactly. It seems that it is a multithreading problem wich is not fully reproducable but increases with more usage of expressions (as shown in sample code).
As we saw the call to AnalyzedArguments.Free in the Conversions class seems to be the problem as the resolution object itself still holds a reference to the AnalyzedArguments object which is used later in the code where the exception is then thrown. Now when the iteration in the OverloadResolutionResult starts with a object which currently has an item in the arguments (cause another thread added an item shortly before) and in the same time another thread calls Free on that object the item gets nulled and we get the exception.
When Free is called the object is get back to the pool and gets reused later which causes the racecondition cause of the reference which is still in the resolution object.
While debugging i found 6-8 threads which shared only 3-4 different AnalyzedArguments objects which just seemed wrong.
When i comment out that call to AnalyzedArguments.Free the code compiles at high memory usage which seems creepy. That's why we decided to open that issue so you can make your professional investigation in that issue... ;)

[jcouv]

@renezweifel Thanks for the awesome analysis!
Indeed, it looks like MethodGroupResolution holds onto instances of AnalyzedArguments, but the codepath that creates those MethodGroupResolution instances also calls analyzedArguments.Free()...

While debugging i found 6-8 threads which shared only 3-4 different AnalyzedArguments objects which just seemed wrong.

Out of curiosity, how did you perform this analysis?
I tried doing this with the "Make object ID" in Visual Studio, but had trouble finding other threads holding onto a specific AnalyzedArguments instance.

[renezweifel]

@jcouv Also with "Make object ID".
Based on the assumption we had we did the following:

1. Create a breakpoint at OverloadResolutionResult.cs line 204 (It crashed there sometimes in the test)
2. Start debugging the test from above
3. Wait until parallelstack looks something like this by hitting "continue" if not (takes some time):
   
4. Go to all threads and click "Make object ID" on every AnalyzedArguments object
5. And the third or fourth time we were at that point we got the following:
   
   
   These 7 Threads share 3 AnalyzedArguments at that moment (object id $4, $5, $6). The screenshots show just 2 of all threads which share the object with the object id $5. One is one out of the two threads left and the other one of the 5 threads right in the parallelstacks screenshot above.

And if one thread modifies that object at a bad time, all crashes.
Another thought we had is that based on this insight, Roslyn will analyze the call to OverloadResolutionResult.HadLambdaConversionError and probably others wrong as there are no or wrong items in the AnalyzedArguments.Arguments collection.
Hopefully that helps to reproduce that condition and create a correct fix (hopefully soon as it's a big issue at our company...).

[renezweifel]

@jcouv Any updates on that issue?

[jcouv]

Not yet. It's going to be a subtle fix, as there is a mix of a few patterns of ownership transfer. May have to resort to reference counting. I'm waiting for a colleague to return from vacation (early Aug) to get some advice.

[jcouv]

Tagging @AlekseyTs @gafter FYI. I'll stop by to discuss (probably tomorrow).

In short: MethodGroupResolution holds onto instances of AnalyzedArguments, but some codepaths that creates those MethodGroupResolution instances also call analyzedArguments.Free().

[renezweifel]

Hi @jcouv
Is it possible to say when the issue will be resolved and in which Visual Studio version it will be included?
It is really a big issue at our company and we would be very happy if it can be fixed as soon as possible.

[jcouv]

Based on the timing and risk, we'd scheduled this for dev16. But from further discussion with Jared, we're going to take a stab for 15.9, assuming we can come up with a relatively low-risk fix (Neal suggested a possible fix which I'll investigate in the next few days).

[jcouv]

Fix proposal (https://github.com/dotnet/roslyn/pull/29360) is in review.

[jcouv]

Fix is merged for 15.9 preview 2.
Thanks @renezweifel for the detailed repro and analysis. That was tremendous help.

[epoyraz]

@jcouv thanks for the fix. i just compiled and tested it with our code base. It works!
Great job 👍