Cancellable: fix leaking cancellation token

[auduchinok]

The previous implementation could lead to the cancellation leaking to another async, leading to unexpected exceptions about disposed cancellation token source.

Thanks a lot to @ForNeVeR for the help with the investigation!

cc @majocha

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.300.md

[majocha]

Yes, good catch. Apparently, I forgot how AsyncLocal works.

[auduchinok]

Could somebody update the ILVerify baselines, please?

[ForNeVeR]

Yes, good catch. Apparently, I forgot how AsyncLocal works.

@majocha you kinda didn't forget and made similar changes in #18285 :)

[majocha]

This is beyond scope of this PR, but it occurred to me now:
The two mechanisms of cancellation of synchronous code, explicit CheckAndThrow and implicit cancellable workflow are now quite independent of each other. The token for CheckAndThrow can be captured without the intermediary cancellable.
It would be good to have some comment in the code explaining how CheckAndThrow works and it's usage, to make it easier to grasp for future readers.

[psfinaki]

@auduchinok @majocha - can you think of any way to test such things?

[auduchinok]

@auduchinok @majocha - can you think of any way to test such things?

Some of the possibly similar cases are tested in the module reader cancellation tests in this repo. They do catch regressions in PRs like this one (e.g. the transparent compiler changes here)

And we also have tests in the Rider plugin. But it's somewhat difficult to come up with test cases for some of the cases, as, for example, this particular case was related to the implementation details of the cancellation token sources handling inside Async.parallel.

This particular leak was reliably reproduced in the tests in the Rider plugin, but unfortunately only when run in the product monorepo. I was not able to reproduce it in the plugin repo or find another repro so far :( Good news are there are tests and environments that can catch such things :)

[majocha]

Something is still wrong, I can repro this error locally
https://dev.azure.com/dnceng-public/public/_build/results?buildId=944083&view=logs&j=09cc464c-89cd-5758-006d-ba9e3958cfa9&t=098678ff-b2f3-5c88-44d3-4c7be2f47e75&l=10832
by simply running all service tests in VS, but it happens randomly.

    CHECKANDTHROW STACKTRACE

   at [email protected](Unit unitVar0)
   at FSharp.Compiler.Cancellable.CheckAndThrow()
   at FSharp.Compiler.Service.Tests.ModuleReaderCancellationTests.runCancelFirstTime@24.Invoke(Unit unitVar0)
   at [email protected](Tuple`2 tupledArg)
   at FSharp.Compiler.Import.multisetDiscriminateAndMap[Key,Value,a](FSharpFunc`2 nodef, FSharpFunc`2 tipf, FSharpList`1 items)
   at FSharp.Compiler.Import.ImportILTypeDefList(FSharpFunc`2 amap, Range m, CompilationPath cpath, FSharpList`1 enc, FSharpList`1 items)
   at [email protected](Unit _arg2)
   at Internal.Utilities.Library.InterruptibleLazy`1.get_Value()
   at Internal.Utilities.Library.Extras.MaybeLazy`1.Force()
   at FSharp.Compiler.TypedTree.Entity.get_IsModule()
   at [email protected](String name, ModuleOrNamespaceType m)
   at [email protected](String name, ModuleOrNamespaceType m)
   at FSharp.Compiler.CompilerImports.addConstraintSources(ImportedAssembly ia)
   at FSharp.Compiler.CompilerImports.TcImports.RegisterAndImportReferencedAssemblies@2356-12.Invoke(ImportedAssembly ia)
   at Microsoft.FSharp.Collections.SeqModule.Iterate[T](FSharpFunc`2 action, IEnumerable`1 source)
   at FSharp.Compiler.CompilerImports.TcImports.RegisterAndImportReferencedAssemblies@2351-9.Invoke(FSharpOption`1[] results)
   at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, b result1, FSharpFunc`2 userCode)
   at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction)
   at <StartupCode$FSharp-Core>[email protected](Object o)
   at System.Threading.QueueUserWorkItemCallback.Execute()
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
   at System.Threading.PortableThreadPool.WorkerThread.WorkerThreadStart()

It's somehow happening in RegisterAndImportReferencedAssemblies.

[majocha]

Yes, now I remember I dealt with it in this experiment:
https://github.com/dotnet/fsharp/pull/18285/files#
It needs yet another UseToken() in RegisterAndImportReferencedAssemblies (around line 2330)
I'm not sure why this is non-deterministic, though.

[auduchinok]

This is ready 🙂

[T-Gro]

do! Cancellable.UseToken() its not used anywhere after this PR, right?

[auduchinok]

do! Cancellable.UseToken() its not used anywhere after this PR, right?

Yes, that's correct

[psfinaki]

Thanks Eugene!