Synchronization bug in `ILPreTypeDefImpl`

[majocha]

Stack trace obtained under multithreaded test run:

[xUnit.net 00:00:01.02]     FSharp.Compiler.UnitTests.SpanTests.Script_ReadOnlySpanForInDo [FAIL]
[xUnit.net 00:00:01.02]       System.Exception : Error creating evaluation session: System.AggregateException: One or more errors occurred. (Value cannot be null.)
[xUnit.net 00:00:01.02]        ---> System.ArgumentNullException: Value cannot be null.
[xUnit.net 00:00:01.02]          at System.Threading.Monitor.Enter(Object obj)
[xUnit.net 00:00:01.02]          at FSharp.Compiler.AbstractIL.IL.ILPreTypeDefImpl.FSharp.Compiler.AbstractIL.IL.ILPreTypeDef.GetTypeDef() in E:\repos\fsharp\src\Compiler\AbstractIL\il.fs:line 2927
[xUnit.net 00:00:01.02]          at FSharp.Compiler.Import.entities@729-1.Invoke(Tuple`2 tupledArg) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 731
[xUnit.net 00:00:01.02]          at FSharp.Compiler.Import.multisetDiscriminateAndMap[Key,Value,a](FSharpFunc`2 nodef, FSharpFunc`2 tipf, FSharpList`1 items) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 689
[xUnit.net 00:00:01.02]          at FSharp.Compiler.Import.ImportILTypeDefList(FSharpFunc`2 amap, Range m, CompilationPath cpath, FSharpList`1 enc, FSharpList`1 items) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 724
[xUnit.net 00:00:01.02]          at FSharp.Compiler.Import.modty@727.Invoke(Unit _arg2) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 727
[xUnit.net 00:00:01.02]          at Internal.Utilities.Library.InterruptibleLazy`1.get_Value() in E:\repos\fsharp\src\Compiler\Utilities\illib.fs:line 33
[xUnit.net 00:00:01.02]          at Internal.Utilities.Library.Extras.MaybeLazy`1.Force() in E:\repos\fsharp\src\Compiler\Utilities\lib.fs:line 396
[xUnit.net 00:00:01.02]          at FSharp.Compiler.CompilerImports.TcImports.ccuHasType(CcuThunk ccu, FSharpList`1 nsname, String tname, Boolean publicOnly) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 1296
[xUnit.net 00:00:01.02]          at FSharp.Compiler.CompilerImports.TcImports.tryFindSysTypeCcu@2596-1.Invoke(CcuThunk ccu) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 2596
[xUnit.net 00:00:01.02]          at Microsoft.FSharp.Collections.ArrayModule.loop@1144-40[T](FSharpFunc`2 predicate, T[] array, Int32 i) in E:\repos\fsharp\src\FSharp.Core\array.fs:line 1147
[xUnit.net 00:00:01.02]          at Microsoft.FSharp.Collections.ArrayModule.TryFind[T](FSharpFunc`2 predicate, T[] array) in E:\repos\fsharp\src\FSharp.Core\array.fs:line 1152
[xUnit.net 00:00:01.02]          at FSharp.Compiler.TcGlobals.TcGlobals.tryFindSysTypeCcu(FSharpList`1 path, String nm) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 216
[xUnit.net 00:00:01.02]          at FSharp.Compiler.TcGlobals.TcGlobals.tryFindSysAttrib(String nm) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 320
[xUnit.net 00:00:01.02]          at FSharp.Compiler.TcGlobals.TcGlobals..ctor(Boolean compilingFSharpCore, ILGlobals ilg, CcuThunk fslibCcu, String directoryToResolveRelativePaths, Boolean mlCompatibility, Boolean isInteractive, Boolean checkNullness, Boolean useReflectionFreeCodeGen, FSharpFunc`2 tryFindSysTypeCcuHelper, Boolean emitDebugInfoInQuotations, Boolean noDebugAttributes, PathMap pathMap, LanguageVersion langVersion, Boolean realsig) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 1079
[xUnit.net 00:00:01.02]          at FSharp.Compiler.CompilerImports.TcImports.BuildFrameworkTcImports@2589-10.Invoke(FSharpList`1 _arg10) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 2602
[xUnit.net 00:00:01.02]          at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, b result1, FSharpFunc`2 userCode) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 528
[xUnit.net 00:00:01.02]          at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 112
[xUnit.net 00:00:01.02]          --- End of inner exception stack trace ---
[xUnit.net 00:00:01.02]          at Internal.Utilities.Library.PervasiveAutoOpens.Async.RunImmediate.Static[T](FSharpAsync`1 computation, FSharpOption`1 cancellationToken) in E:\repos\fsharp\src\Compiler\Utilities\illib.fs:line 151
[xUnit.net 00:00:01.02]          at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession..ctor(FsiEvaluationSessionHostConfig fsi, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, Boolean fsiCollectible, FSharpOption`1 legacyReferenceResolver) in E:\repos\fsharp\src\Compiler\Interactive\fsi.fs:line 4673
[xUnit.net 00:00:01.02]       Stack Trace:
[xUnit.net 00:00:01.02]            at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1448.Invoke(String message)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\src\Compiler\Interactive\fsi.fs(4676,0): at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession..ctor(FsiEvaluationSessionHostConfig fsi, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, Boolean fsiCollectible, FSharpOption`1 legacyReferenceResolver)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\src\Compiler\Interactive\fsi.fs(5007,0): at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession.Create(FsiEvaluationSessionHostConfig fsiConfig, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, FSharpOption`1 collectible, FSharpOption`1 legacyReferenceResolver)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\tests\FSharp.Test.Utilities\CompilerAssert.fs(1004,0): at FSharp.Test.CompilerAssert.RunScriptWithOptionsAndReturnResult(String[] options, String source)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\tests\FSharp.Test.Utilities\CompilerAssert.fs(1019,0): at FSharp.Test.CompilerAssert.RunScriptWithOptions(String[] options, String source, FSharpList`1 expectedErrorMessages)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\tests\FSharp.Test.Utilities\CompilerAssert.fs(1029,0): at FSharp.Test.CompilerAssert.RunScript(String source, FSharpList`1 expectedErrorMessages)
[xUnit.net 00:00:01.02]         E:\repos\fsharp\tests\fsharp\Compiler\Language\SpanTests.fs(91,0): at FSharp.Compiler.UnitTests.SpanTests.Script_ReadOnlySpanForInDo()
[xUnit.net 00:00:01.02]            at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
[xUnit.net 00:00:01.02]            at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)
[xUnit.net 00:00:05.32]       failing on main

The code in question:

fsharp/src/Compiler/AbstractIL/il.fs

Lines 2914 to 2944 in 05cf886

	/// This is a memory-critical class. Very many of these objects get allocated and held to represent the contents of .NET assemblies.
	and [<Sealed>] ILPreTypeDefImpl(nameSpace: string list, name: string, metadataIndex: int32, storage: ILTypeDefStored) =
	let mutable store: ILTypeDef = Unchecked.defaultof<_>
	let mutable storage = storage
	interface ILPreTypeDef with
	member _.Namespace = nameSpace
	member _.Name = name
	member x.GetTypeDef() =
	match box store with
	| null ->
	let syncObj = storage
	Monitor.Enter(syncObj)
	try
	match box store with
	| null ->
	let value =
	match storage with
	| ILTypeDefStored.Given td -> td
	| ILTypeDefStored.Computed f -> f ()
	| ILTypeDefStored.Reader f -> f metadataIndex
	store <- value
	storage <- Unchecked.defaultof<_>
	value
	| _ -> store
	finally
	Monitor.Exit(syncObj)
	| _ -> store

[majocha]

                 let syncObj = storage 
                 Monitor.Enter(syncObj) 

While storage is getting assigned null under lock, I see no guarantee syncObj is not already null when we try to enter the monitor.

[majocha]

Another one just now, in case it's different.

      Miscellaneous.FsharpSuiteMigrated_CoreTests.Tests4.members-ctree-FSC_DEBUG (867ms): Error Message: System.Exception : Error creating evaluation session: System.AggregateException: One or more er
      rors occurred. (Value cannot be null.)
       ---> System.ArgumentNullException: Value cannot be null.
         at System.Threading.Monitor.Enter(Object obj)
         at FSharp.Compiler.AbstractIL.IL.ILPreTypeDefImpl.FSharp.Compiler.AbstractIL.IL.ILPreTypeDef.GetTypeDef() in E:\repos\fsharp\src\Compiler\AbstractIL\il.fs:line 2927
         at FSharp.Compiler.Import.entities@729-1.Invoke(Tuple`2 tupledArg) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 731
         at FSharp.Compiler.Import.multisetDiscriminateAndMap[Key,Value,a](FSharpFunc`2 nodef, FSharpFunc`2 tipf, FSharpList`1 items) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 689
         at FSharp.Compiler.Import.ImportILTypeDefList(FSharpFunc`2 amap, Range m, CompilationPath cpath, FSharpList`1 enc, FSharpList`1 items) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line
      724
         at FSharp.Compiler.Import.modty@727.Invoke(Unit _arg2) in E:\repos\fsharp\src\Compiler\Checking\import.fs:line 727
         at Internal.Utilities.Library.InterruptibleLazy`1.get_Value() in E:\repos\fsharp\src\Compiler\Utilities\illib.fs:line 33
         at Internal.Utilities.Library.Extras.MaybeLazy`1.Force() in E:\repos\fsharp\src\Compiler\Utilities\lib.fs:line 396
         at FSharp.Compiler.CompilerImports.TcImports.ccuHasType(CcuThunk ccu, FSharpList`1 nsname, String tname, Boolean publicOnly) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 129
      6
         at FSharp.Compiler.CompilerImports.TcImports.tryFindSysTypeCcu@2596-1.Invoke(CcuThunk ccu) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 2596
         at Microsoft.FSharp.Collections.ArrayModule.loop@1144-40[T](FSharpFunc`2 predicate, T[] array, Int32 i) in E:\repos\fsharp\src\FSharp.Core\array.fs:line 1147
         at Microsoft.FSharp.Collections.ArrayModule.TryFind[T](FSharpFunc`2 predicate, T[] array) in E:\repos\fsharp\src\FSharp.Core\array.fs:line 1152
         at FSharp.Compiler.TcGlobals.TcGlobals.tryFindSysTypeCcu(FSharpList`1 path, String nm) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 216
         at FSharp.Compiler.TcGlobals.TcGlobals.tryFindSysAttrib(String nm) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 320
         at FSharp.Compiler.TcGlobals.TcGlobals..ctor(Boolean compilingFSharpCore, ILGlobals ilg, CcuThunk fslibCcu, String directoryToResolveRelativePaths, Boolean mlCompatibility, Boolean isInteract
      ive, Boolean checkNullness, Boolean useReflectionFreeCodeGen, FSharpFunc`2 tryFindSysTypeCcuHelper, Boolean emitDebugInfoInQuotations, Boolean noDebugAttributes, PathMap pathMap, LanguageVersion
       langVersion, Boolean realsig) in E:\repos\fsharp\src\Compiler\TypedTree\TcGlobals.fs:line 1079
         at FSharp.Compiler.CompilerImports.TcImports.BuildFrameworkTcImports@2589-10.Invoke(FSharpList`1 _arg10) in E:\repos\fsharp\src\Compiler\Driver\CompilerImports.fs:line 2602
         at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, b result1, FSharpFunc`2 userCode) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 528
         at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in E:\repos\fsharp\src\FSharp.Core\async.fs:line 112
         --- End of inner exception stack trace ---
         at Internal.Utilities.Library.PervasiveAutoOpens.Async.RunImmediate.Static[T](FSharpAsync`1 computation, FSharpOption`1 cancellationToken) in E:\repos\fsharp\src\Compiler\Utilities\illib.fs:l
      ine 151
         at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession..ctor(FsiEvaluationSessionHostConfig fsi, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, Boolean f
      siCollectible, FSharpOption`1 legacyReferenceResolver) in E:\repos\fsharp\src\Compiler\Interactive\fsi.fs:line 4673
      Stack Trace:
         at Microsoft.FSharp.Core.PrintfModule.PrintFormatToStringThenFail@1448.Invoke(String message)
         at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession..ctor(FsiEvaluationSessionHostConfig fsi, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, Boolean f
      siCollectible, FSharpOption`1 legacyReferenceResolver) in E:\repos\fsharp\src\Compiler\Interactive\fsi.fs:line 4676
         at FSharp.Compiler.Interactive.Shell.FsiEvaluationSession.Create(FsiEvaluationSessionHostConfig fsiConfig, String[] argv, TextReader inReader, TextWriter outWriter, TextWriter errorWriter, FS
      harpOption`1 collectible, FSharpOption`1 legacyReferenceResolver) in E:\repos\fsharp\src\Compiler\Interactive\fsi.fs:line 5007
         at FSharp.Test.ScriptHelpers.FSharpScript..ctor(FSharpOption`1 additionalArgs, FSharpOption`1 quiet, FSharpOption`1 langVersion, FSharpOption`1 input) in E:\repos\fsharp\tests\FSharp.Test.Uti
      lities\ScriptHelpers.fs:line 84
         at FSharp.Test.Compiler.getSessionForEval(String[] args, LangVersion version) in E:\repos\fsharp\tests\FSharp.Test.Utilities\Compiler.fs:line 1044
         at Miscellaneous.FsharpSuiteMigrated.ScriptRunner.createEngine(String[] args, LangVersion version) in E:\repos\fsharp\tests\FSharp.Compiler.ComponentTests\Miscellaneous\FsharpSuiteMigrated.fs
      :line 21
         at Miscellaneous.FsharpSuiteMigrated.ScriptRunner.runScriptFile(LangVersion version, CompilationUnit cu) in E:\repos\fsharp\tests\FSharp.Compiler.ComponentTests\Miscellaneous\FsharpSuiteMigra
      ted.fs:line 34
         at Miscellaneous.FsharpSuiteMigrated.TestFrameworkAdapter.singleTestBuildAndRunAuxVersion(String folder, FSharpList`1 bonusArgs, ExecutionMode mode, LangVersion langVersion) in E:\repos\fshar
      p\tests\FSharp.Compiler.ComponentTests\Miscellaneous\FsharpSuiteMigrated.fs:line 138
         at Miscellaneous.FsharpSuiteMigrated.TestFrameworkAdapter.singleTestBuildAndRunVersion(String folder, ExecutionMode mode, LangVersion version) in E:\repos\fsharp\tests\FSharp.Compiler.Compone
      ntTests\Miscellaneous\FsharpSuiteMigrated.fs:line 161
         at Miscellaneous.FsharpSuiteMigrated.TestFrameworkAdapter.singleTestBuildAndRun(String folder, ExecutionMode mode) in E:\repos\fsharp\tests\FSharp.Compiler.ComponentTests\Miscellaneous\Fsharp
      SuiteMigrated.fs:line 162
         at Miscellaneous.FsharpSuiteMigrated_CoreTests.Tests4.members-ctree-FSC_DEBUG() in E:\repos\fsharp\tests\FSharp.Compiler.ComponentTests\Miscellaneous\MigratedCoreTests.fs:line 273
         at System.RuntimeMethodHandle.InvokeMethod(Object target, Void** arguments, Signature sig, Boolean isConstructor)
         at System.Reflection.MethodBaseInvoker.InvokeWithNoArgs(Object obj, BindingFlags invokeAttr)

[vzarytovskii]

Hm, is it something recent? Haven't seen it yet. Or is it reproducing only under certain conditions?

[majocha]

I could repro this in #17662 from the beginning, in various forms and situations, but the exception didn't surface, just FS0193 "Internal error: value cannot be null".
This is the first time I hit a jackpot of a stack trace.

[majocha]

I just replaced this code in #17662 with simple lazy. Just to see if it improves things. I'll look at the results tomorrow.

[majocha]

Simple System.Lazy<_> seems to work fine.

/// This is a memory-critical class. Very many of these objects get allocated and held to represent the contents of .NET assemblies.
and [<Sealed>] ILPreTypeDefImpl(nameSpace: string list, name: string, metadataIndex: int32, storage: ILTypeDefStored) =
    let onDemand =
        lazy
        match storage with
        | ILTypeDefStored.Given td -> td
        | ILTypeDefStored.Computed f -> f ()
        | ILTypeDefStored.Reader f -> f metadataIndex

    interface ILPreTypeDef with
        member _.Namespace = nameSpace
        member _.Name = name
        member x.GetTypeDef() = onDemand.Value

Or would Lazy<_>(... , LazyThreadSafetyMode.PublicationOnly) perform better here?
I guess this does not need InterruptibleLazy?

@auduchinok, could you please take a look when you have time?

[auduchinok]

Thanks a lot for the investigation @majocha!

Or would Lazy<_>(... , LazyThreadSafetyMode.PublicationOnly) perform better here?

It would cache exceptions then, and that's what we wanted to avoid with InterruptibleLazy.

We don't use ILPreTypeDefImpl, so it's fine to change this particular type (as we don't expect OCE being thrown here), but I think that the main thing to do is to fix InterruptibleLazy, as the same issue can happen in other places.

[majocha]

I think PublicationOnly doesn't cache exceptions, but also doesn't guarantee single execution:

From my understanding InterruptibleLazy fills the not covered mode of guaranteed single execution but without caching exceptions?

I tried the fix from previous comment and it seems to work fine locally and in #17662.

[auduchinok]

From my understanding InterruptibleLazy fills the not covered mode of guaranteed single execution but without caching exceptions?

Exactly. 🙂