Symbols API: full thread-safety

[auduchinok]

There're some APIs using which from multiple threads at the same time may lead to a dead lock or to an infinite looping.
I've seen this in several places and when attaching a debugger to a hanging process, it was hanging inside Dictionary`2. (part of parallels stack screenshot is attached below).

I've seen it in FSharpEntity.BaseType, FSharpMemberOrFunctionOrValue.XmlDocSig and now in FSharpChecker.GetProjectOptionsFromScript(...). Adding locks around uses of the first two APIs fixes the the issue for me. If needed I can remove the locks to reproduce it and provide stack traces.

Can we replace uses of Dictionary with ConcurrentDictionary in places that may be (indirectly) called by FCS clients?

[dsyme]

I've seen it in FSharpEntity.BaseType, FSharpMemberOrFunctionOrValue.XmlDocSig and now in FSharpChecker.GetProjectOptionsFromScript(...). Adding locks around uses of the first two APIs fixes the the issue for me. If needed I can remove the locks to reproduce it and provide stack traces.

Ugh. Yes. For the specific stacktraces above that state should either be ConcurrentDictionary or should not be being accessed concurrently. I'd like to assess what's gone wrong with our ctok methodology that allowed this to slip through.

If you can get additional stack traces I'll definitely take a close look at these, we have to root out these cases.

[auduchinok]

A different situation that seems related:

--- EXCEPTION #1/3 [ArgumentException]
Message = “An item with the same key has already been added.”
ExceptionPath.1 = Root.InnerException.InnerException
ExceptionPath.2 = Root.InnerException.InnerExceptions.#0
ClassName = System.ArgumentException
HResult = E_INVALIDARG=COR_E_ARGUMENT=80070057
Source = mscorlib
StackTraceString = “
  at System.ThrowHelper.ThrowArgumentException(ExceptionResource resource)
     at System.Collections.Generic.Dictionary`2.Insert(TKey key, TValue value, Boolean add)
     at Microsoft.FSharp.Compiler.CompileOps.DeduplicateParsedInputModuleName(Dictionary`2 moduleNamesDict, ParsedInput input)
     at <StartupCode$FSharp-Compiler-Service>.$Service.parseTreeOpt@2626-1.Invoke(ParsedInput arg00)
     at Microsoft.FSharp.Core.OptionModule.Map[T,TResult](FSharpFunc`2 mapping, FSharpOption`1 option)
     at <StartupCode$FSharp-Compiler-Service>.$Service.ParseAndCheckFileInProject@2624-7.Invoke(PartialCheckResults _arg20)
     at Microsoft.FSharp.Control.AsyncBuilderImpl.args@823-1.Invoke(a a)
”

--- Outer ---

--- EXCEPTION #2/3 [AggregateException]
Message = “One or more errors occurred.”
ExceptionPath = Root.InnerException
ClassName = System.AggregateException
InnerException = “Exception #1 at Root.InnerException.InnerException”
HResult = COR_E_EXCEPTION=80131500
Source = mscorlib
InnerExceptions.#0 = “Exception #1 at Root.InnerException.InnerException”
StackTraceString = “
  at System.Threading.Tasks.Task.ThrowIfExceptional(Boolean includeTaskCanceledExceptions)
     at System.Threading.Tasks.Task.Wait(Int32 millisecondsTimeout, CancellationToken cancellationToken)
     at JetBrains.ReSharper.Plugins.FSharp.Common.Util.CommonUtil.Async`1.RunAsTask[T](FSharpAsync`1 x, FSharpOption`1 interruptChecker)
     at JetBrains.ReSharper.Plugins.FSharp.Common.Checker.FSharpCheckerService.ParseAndCheckFile(IPsiSourceFile file, Boolean allowStaleResults)
     at JetBrains.ReSharper.Plugins.FSharp.Psi.Impl.Tree.FSharpFileBase.GetParseAndCheckResults(Boolean allowStaleResults, Action interruptChecker)
     at JetBrains.ReSharper.Plugins.FSharp.Psi.Features.CodeCompletion.FSharpLookupItemsProviderBase.AddLookupItems(FSharpCodeCompletionContext context, IItemsCollector collector)
     at JetBrains.ReSharper.Plugins.FSharp.Psi.Features.CodeCompletion.FSharpLookupItemsProvider.JetBrains-ReSharper-Feature-Services-CodeCompletion-Infrastructure-ICodeCompletionItemsProvider-AddLookupItems(ISpecificCodeCompletionContext context, IItemsCollector collector, Object _arg1)
     at JetBrains.Util.Logging.Logger.CatchIgnoreException[TValue,TException](Func`1 F, TValue defaultValue) in c:\Build Agent\work\aaa50a9152947b10\Platform\Core\Shell\Core\Src\Logging\Logger.cs:line 549
”

[dsyme]

@auduchinok Yes, that is related. We should move to ConcurrentDictionary - do you plan to submit a PR? Thanks

[auduchinok]

The exception above was an about 100% repro in our test suite with a simple file:

// Learn more about F# at http://fsharp.org

open System

[<EntryPoint>]
let main argv =
    printfn "Hello World from F#!"
    List.{caret}
    0 // return an integer exit code

and project options:

{ProjectFileName =
    "C:\Users\Eugene\AppData\Local\Temp\JetTestRider\205028318\EditorTest\TestDir.fsproj";
   SourceFiles =
    [|"C:\Users\Eugene\AppData\Local\Temp\JetTestRider\205028318\EditorTest\Program.fs"|];
   OtherOptions =
    [|"--out:C:\Users\Eugene\AppData\Local\Temp\JetTestRider\205028318\EditorTest\bin\Debug\netcoreapp2.0\TestDir.dll";
      "--noframework"; "--debug:full"; "--debug+"; "--optimize-"; "--tailcalls-";
      "--fullpaths"; "--flaterrors"; "--highentropyva+"; "--target:exe";
      "-r:C:\Users\Eugene\.nuget\packages\fsharp.core\4.2.3\lib\netstandard1.6\FSharp.Core.dll";
      "-r:C:\Users\Eugene\.nuget\packages\microsoft.netcore.app\2.0.0\ref\netcoreapp2.0\Microsoft.CSharp.dll";
      "-r:C:\Users\Eugene\.nuget\packages\microsoft.netcore.app\2.0.0\ref\netcoreapp2.0\Microsoft.VisualBasic.dll";
      "-r:C:\Users\Eugene\.nuget\packages\microsoft.netcore.app\2.0.0\ref\netcoreapp2.0\Microsoft.Win32.Primitives.dll";

      // a long list of references here

      ...|];
   ReferencedProjects = [||];
   IsIncompleteTypeCheckEnvironment = false;
   UseScriptResolutionRules = false;
   LoadTime = 22.11.2017 20:51:05;
   UnresolvedReferences = None;
   OriginalLoadReferences = [];
   ExtraProjectInfo = None;
   Stamp = None;}

The test opens a file editor in Rider, moves the caret, types in a text to complete and completes it. At about the same time ReSharper builds its cache in background threads with mapping FCS types to its R# representations (and calculating things like base types where applicable, checking attributes, etc); at the same time analysis asks for syntax/type check errors, identifiers highlighting, etc. Almost all of these activities rely on file type check results (and Symbols API) and happen in multiple threads in which ReSharper calls them.

I think I've faced a freeze during this method calls before, however I was never able to reproduce it reliably like in this test. Adding a lock around ParseAndCheckFileInProject fixes it for me making the test pass, however I really doubt adding this lock to the production code is a good idea.

@dsyme Sure, I'll open a PR. I'll try removing other locks as I've told before and open additional PRs/issues if needed.

[auduchinok]

Another example is from ImportMap:

@dsyme Would it be safe to swap the Dictionary with the concurrent one here as well?

[dsyme]

@auduchinok These are both good fixes.

However the range of micro operations on the FCS Symbol API are clearly now leading us to race conditions that we weren't previously exposed to, particularly when exploring "previously unexplored" information in the symbol graph. Rider must be exploiting concurrent access to more information in symbols more than VS, or else your tools are just better at identifying race conditions

I'll adjust this bug to be a tracking bug to work out a systematic approach to eliminating these races, either through

1. the use of concurrent data structures (i.e. fine-grained locks)
2. the elimination of mutable state from the symbols
3. the use of coarse-grained locks (i.e. the reactor thread or a global lock)

thanks