Skip to content

Set Cancellable.token from async computation - #18238

Merged
T-Gro merged 7 commits into
dotnet:mainfrom
majocha:fix-18235
Jan 16, 2025
Merged

Set Cancellable.token from async computation#18238
T-Gro merged 7 commits into
dotnet:mainfrom
majocha:fix-18235

Conversation

@majocha

@majochamajocha commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

Fixes#18235

@github-actions

github-actionsBot commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change pathRelease notes pathDescription
src/Compilerdocs/release-notes/.FSharp.Compiler.Service/9.0.300.md

@majocha

Copy link
Copy Markdown
ContributorAuthor

@auduchinok pls take a look.

Searching for ILModuleDef I see there are a few other calls that are not obvious for me. So this may not fix the problem entirely.

@majocha

Copy link
Copy Markdown
ContributorAuthor

I wonder if we could remove the cancellable CE altogether and just capture the cancellation token into AsyncLocal at all async service endpoints instead.

@auduchinok

Copy link
Copy Markdown
Member

I wonder if we could remove the cancellable CE altogether and just capture the cancellation token into AsyncLocal at all async service endpoints instead.

Sounds good! I think @vzarytovskii had a similar idea some time ago.

@majocha

Copy link
Copy Markdown
ContributorAuthor

I wonder if we could remove the cancellable CE altogether and just capture the cancellation token into AsyncLocal at all async service endpoints instead.

Sounds good! I think @vzarytovskii had a similar idea some time ago.

That would probably require converting all the cancellable uses to async, keeping in sight the goal of replacing async with more performant resumable CE with better stack traces (?).

@auduchinok

Copy link
Copy Markdown
Member

@majocha This is from the same test with this PR applied:

System.Exception: Token not available outside of Cancellable computation.
at <StartupCode$FSharp-Compiler-Service>.$Cancellable.defThunk@1(String msg, Unit unitVar0) in C:\Developer\jetbrains-fcs\src\Compiler\Utilities\Cancellable.fs:line 15
at FSharp.Compiler.Cancellable.ensureToken(String msg) in C:\Developer\jetbrains-fcs\src\Compiler\Utilities\Cancellable.fs:line 14
at FSharp.Compiler.Cancellable.get_Token() in C:\Developer\jetbrains-fcs\src\Compiler\Utilities\Cancellable.fs:line 19
at JetBrains.ReSharper.Plugins.FSharp.FSharpAsyncUtil.UsingReadLockInsideFcs(IShellLocks locks, Action action, Func`1 upToDateCheck) in C:\Developer\resharper-fsharp\ReSharper.FSharp\src\FSharp\FSharp.ProjectModelBase\src\FSharpAsyncUtil.cs:line 40
at JetBrains.ReSharper.Plugins.FSharp.Shim.AssemblyReader.ProjectFcsModuleReader.readData(FSharpFunc`2 f) in C:\Developer\resharper-fsharp\ReSharper.FSharp\src\FSharp\FSharp.Common\src\Shim\AssemblyReader\ProjectFcsModuleReader.fs:line 142
at JetBrains.ReSharper.Plugins.FSharp.Shim.AssemblyReader.ProjectFcsModuleReader.CreateTypeDef(IClrTypeName clrTypeName) in C:\Developer\resharper-fsharp\ReSharper.FSharp\src\FSharp\FSharp.Common\src\Shim\AssemblyReader\ProjectFcsModuleReader.fs:line 1273
at JetBrains.ReSharper.Plugins.FSharp.Shim.AssemblyReader.PreTypeDef.FSharp.Compiler.AbstractIL.IL.ILPreTypeDef.GetTypeDef() in C:\Developer\resharper-fsharp\ReSharper.FSharp\src\FSharp\FSharp.Common\src\Shim\AssemblyReader\ProjectFcsModuleReader.fs:line 1484
at FSharp.Compiler.Import.entities@784-1.Invoke(Tuple`2 tupledArg) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 786
at FSharp.Compiler.Import.multisetDiscriminateAndMap[Key,Value,a](FSharpFunc`2 nodef, FSharpFunc`2 tipf, FSharpList`1 items) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 726
at FSharp.Compiler.Import.ImportILTypeDefList(FSharpFunc`2 amap, Range m, CompilationPath cpath, FSharpList`1 enc, FSharpList`1 items) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 779
at FSharp.Compiler.Import.ImportILTypeDefs(FSharpFunc`2 amap, Range m, ILScopeRef scoref, CompilationPath cpath, FSharpList`1 enc, ILTypeDefs tdefs) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 796
at FSharp.Compiler.Import.ImportILAssemblyMainTypeDefs(FSharpFunc`2 amap, Range m, ILScopeRef scoref, ILModuleDef modul) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 806
at FSharp.Compiler.Import.ImportILAssemblyTypeDefs(FSharpFunc`2 amap, Range m, FSharpFunc`2 auxModLoader, ILAssemblyRef aref, ILModuleDef mainmod) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 842
at FSharp.Compiler.Import.ImportILAssembly(FSharpFunc`2 amap, Range m, FSharpFunc`2 auxModuleLoader, FSharpOption`1 xmlDocInfoLoader, ILScopeRef ilScopeRef, String sourceDir, FSharpOption`1 fileName, ILModuleDef ilModule, IEvent`2 invalidateCcu) in C:\Developer\jetbrains-fcs\src\Compiler\Checking\import.fs:line 908
at FSharp.Compiler.CompilerImports.TcImports.PrepareToImportReferencedILAssembly(CompilationThreadToken ctok, Range m, String fileName, ImportedBinary dllinfo) in C:\Developer\jetbrains-fcs\src\Compiler\Driver\CompilerImports.fs:line 2058
at FSharp.Compiler.CompilerImports.TryRegisterAndPrepareToImportReferencedDll@2279-8.Invoke(IRawFSharpAssemblyData assemblyData) in C:\Developer\jetbrains-fcs\src\Compiler\Driver\CompilerImports.fs:line 2316
at Microsoft.FSharp.Control.AsyncPrimitives.CallThenInvokeNoHijackCheck[a,b](AsyncActivation`1 ctxt, b result1, FSharpFunc`2 userCode) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 528
at Microsoft.FSharp.Control.Trampoline.Execute(FSharpFunc`2 firstAction) in D:\a\_work\1\s\src\FSharp.Core\async.fs:line 112

@majocha

majocha commented Jan 15, 2025

Copy link
Copy Markdown
ContributorAuthor

Yes, I have another idea. Just define Cancellable.UseToken: Async<unit> that will sync the Cancellable token with current async token.

@auduchinok

Copy link
Copy Markdown
Member

@majocha Yes, with this patch all the tests are green 🎉

@majochamajocha changed the title Wrap ILModuleReader with cancellableSet Cancellable.token from async computationJan 15, 2025
@majocha
majocha marked this pull request as ready for review January 15, 2025 15:16
@majocha
majocha requested a review from a team as a code ownerJanuary 15, 2025 15:16
@T-Gro
T-Gro enabled auto-merge (squash) January 16, 2025 13:20

@psfinakipsfinaki left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@T-Gro
T-Gro merged commit 8473ce6 into dotnet:mainJan 16, 2025
@majocha
majocha deleted the fix-18235 branch January 16, 2025 15:18
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Non-cancellable metadata access when importing a DLL

4 participants

@majocha@auduchinok@psfinaki@T-Gro