Sequentialise type provider calls by using a lock rather than requiring the Reactor single threading

[TIHan]

This forces type provider calls to not ever happen in parallel.

[auduchinok]

Just a thought: have you considered having a locking mechanism for TP calls instead of queueing them to the reactor thread? It could allow accessing TPs while processing other requests in the reactor, while guaranteeing single-thread access.

[TIHan]

@auduchinok I haven't thought about it, but I guess it could work? Talking with Don now, we think it should work - and it would make this implementation simpler :).

[TIHan]

Yea, I'm going to remove ICompilationThread entirely and replace it with a Lock object that we have.

[TIHan]

Thank you for the suggestion @auduchinok . It's going to make solving this issue much easier than what I tried to do. :)

[dsyme]

Strange error in CI, it looks more related to "FSharp.Compiler.Interactive.Settings" not being referenced or something like that

2020-10-22T20:26:12.2724722Z   X fsiShouldTriggerCompletionInFsxFile [1m 2s]
2020-10-22T20:26:12.2727256Z   Error Message:
2020-10-22T20:26:12.2728161Z    Expected:
2020-10-22T20:26:12.2729180Z "CommandLineArgs"; "EventLoop"; "FloatingPointFormat"; "FormatProvider"; "PrintDepth"; "PrintLength"; "PrintSize"; "PrintWidth"; "ShowDeclarationValues"; "ShowIEnumerable"; "ShowProperties"; "AddPrinter"; "AddPrintTransformer"; "Equals"; "GetHashCode"; "GetType"; "ToString",
2020-10-22T20:26:12.2730679Z but was:
2020-10-22T20:26:12.2731147Z 
2020-10-22T20:26:12.2732037Z actual with sort text:
2020-10-22T20:26:12.2732662Z 
2020-10-22T20:26:12.2733254Z   Stack Trace:
2020-10-22T20:26:12.2734257Z      at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.Worker.VerifyCompletionListExactly(String fileContents, String marker, List`1 expected) in D:\a\1\s\vsintegration\tests\UnitTests\FsxCompletionProviderTests.fs:line 124
2020-10-22T20:26:12.2735539Z    at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.Worker.VerifyCompletionListExactly(String fileContents, String marker, List`1 expected)
2020-10-22T20:26:12.2736754Z    at Microsoft.VisualStudio.FSharp.Editor.Tests.Roslyn.FsxCompletionProviderTests.fsiShouldTriggerCompletionInFsxFile() in D:\a\1\s\vsintegration\tests\UnitTests\FsxCompletionProviderTests.fs:line 150
2020-10-22T20:26:12.2737650Z 

[TIHan]

Yea, I'm looking at what is going on.

[dsyme]

This forces type provider calls to not ever happen in parallel.

Note: this is currently achieved by having the compiler be single threaded through the Reactor thread. However we can hardwire the serialization of TP calls in at a lower level through locking on each TP call.

context.TypeProvider

@TIHan Do you think we should associate the lock with the TypeProvider instance rather than making it global? Or at least scope it per project or per solution?

In particular, if a TP call doesn't return of hang, this will currently stop analysis for all projects, all solutions. By associating it with the type provider instance we get the result we need (serialize requests to each particular type provider instance). And if a TP call hangs the user can still continue working in other scripts and projects with the TP call outstanding in the background. There will be problems, e.g. resources will gradually leak associated these scripts and projects, but it seems better to scope it?

[dsyme]

That is, we would create one lock object here and put it in each TaintedContext. This routine is called once for each referenced Type Provider Design Time Component in each script/project (i.e. in TcImports). So each TPDTC would get its requests serialized for each time it is used.

    static member CreateAll(providerSpecs : (ITypeProvider * ILScopeRef) list) =
        [for (tp,nm) in providerSpecs do
             yield Tainted<_>({ TypeProvider=tp; TypeProviderAssemblyRef=nm },tp) ] 

[dsyme]

Also please remove this code in Import.fs

    // Explanation: The two calls below represent am unchecked invariant of the hosted compiler: 
    // that type providers are only activated on the CompilationThread. This invariant is not currently checked 
    // via CompilationThreadToken passing. We leave the two calls below as a reminder of this.
    //
    // This function is one major source of type provider activations, but not the only one: almost 
    // any call in the 'ExtensionTyping' module is a potential type provider activation.
    let ctok = AssumeCompilationThreadWithoutEvidence ()
    RequireCompilationThread ctok

[dsyme]

@TIHan I'm sure that test failure is unrelated?

[TIHan]

Do you think we should associate the lock with the TypeProvider instance rather than making it global? Or at least scope it per project or per solution?

Per type provider, maybe? You could have a case where you have two type providers that share some global state that isn't concurrently safe. That would be my fear. Today all type providers across all projects, their operations never occur in parallel of one another.

I suggest we try to force all TPs to be single-threaded, even if they hang - it won't regress since they hang today. Just less risk of not accounting for something that could happen inside of multiple TP instances. We are already making pretty significant changes with the reactor; let's get all of that working and safe, then we can evaluate the TPs more.

[dsyme]

Per type provider, maybe? You could have a case where you have two type providers that share some global state that isn't concurrently safe. That would be my fear. Today all type providers across all projects, their operations never occur in parallel of one another.

You mean per TP DLL? I mean, yes, this is a possible choice. However if multiple scripts or projects share the same type provider they will all be hosed by a single long call.

I think we should make it one-lock-per-DLL-per-(project/script/TcImports), i.e. what I say above. That feels right to me. TPs are the logical equivalent of MSBuild tasks. As I understand it different instances of MSBuild tasks may be called from multiple threads and that's just how the programming extension point works, and it was always how TPs were intended, and always what we advertised.

If TPs are breaking the rules and storing global static state then that is mistaken programming on the part of the TP author. TPs fundamentally may be instantiated with different logical arguments by different compilation instances. They just shouldn't be sharing global state between those instances.

[dsyme]

@TIHan If it helps, I think in reality this PR improves over the current situation in any case.

Currently, the intention is that anything requiring TP invocation goes via the Reactor thread. However many of the Symbols.fsi/fs operations require TP access and we don't go through the Reactor thread for each and every underlying MethInfo operation, for example. So this PR will correct that situation.

So I believe that in practice TPs instances are currently being hit concurrently from VS tooling, though in the vast majority of cases this causes no data races and no problems, because nearly all the information being accessed is either in functional data structures, or the ProvidedType/MethodInfo etc. instances, or is invariant in any case.

So this PR corrects that, which is great and will at the very minimum sequentialise access per-TP-DLL-per-project. For this reason I think we are free to decide the granularity of sequentialization as part of this PR - whatever we do will only improve things.

[TIHan]

TPs instances are currently being hit concurrently from VS tooling

Hmm, if that is the case, then we could be safe in doing a lock per TP instance.

For this reason I think we are free to decide the granularity of sequentialization as part of this PR

I would rather do a lock per TP instance; I just don't know all the implications of it without spending time looking at how TPs are called. If you think it's safe to do, I can make a lock per TP instance to see what our CI says. My reasoning with the single lock was simply to reduce risk and not change too much.

[cartermp]

The approach simplifies the implementation and improves the intended experience for TP calls. Love it!

[dsyme]

This is great, I will merge