Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Non-cancellable metadata access when importing a DLL #18235

Open
auduchinok opened this issue Jan 14, 2025 · 8 comments · May be fixed by #18238
Open

Non-cancellable metadata access when importing a DLL #18235

auduchinok opened this issue Jan 14, 2025 · 8 comments · May be fixed by #18238
Labels
Bug Needs-Triage
Milestone
Backlog

Comments

@auduchinok
Copy link
Member

auduchinok commented Jan 14, 2025 •
edited
Loading 

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-fsharp\src\Compiler\Utilities\Cancellable.fs:line 15
   at FSharp.Compiler.Cancellable.ensureToken(String msg) in C:\Developer\jetbrains-fsharp\src\Compiler\Utilities\Cancellable.fs:line 14
   at FSharp.Compiler.Cancellable.get_Token() in C:\Developer\jetbrains-fsharp\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.FSharp.Compiler.AbstractIL.ILBinaryReader.ILModuleReader.get_ILModuleDef() in C:\Developer\resharper-fsharp\ReSharper.FSharp\src\FSharp\FSharp.Common\src\Shim\AssemblyReader\ProjectFcsModuleReader.fs:line 1358
   at FSharp.Compiler.CompilerImports.TcImports.OpenILBinaryModule(CompilationThreadToken ctok, String fileName, Range m) in C:\Developer\jetbrains-fsharp\src\Compiler\Driver\CompilerImports.fs:line 1635
And this is the stack from the debugger: 
$Cancellable.defThunk@1() at C:/Developer/jetbrains-fsharp/src/Compiler/Utilities/Cancellable.fs:line 15
Cancellable.ensureToken() at C:/Developer/jetbrains-fsharp/src/Compiler/Utilities/Cancellable.fs:line 14
Cancellable.get_Token()
FSharpAsyncUtil.UsingReadLockInsideFcs()
ProjectFcsModuleReader.readData()
ProjectFcsModuleReader.FSharp.Compiler.AbstractIL.ILBinaryReader.ILModuleReader.get_ILModuleDef()
CompilerImports.TcImports.OpenILBinaryModule()
CompilerImports.TryRegisterAndPrepareToImportReferencedDll@2265-3.Invoke()
AsyncPrimitives.CallThenInvokeNoHijackCheck<Microsoft.FSharp.Core.FSharpOption<System.Tuple<FSharp.Compiler.CompilerImports.ImportedBinary, Microsoft.FSharp.Core.FSharpFunc<Microsoft.FSharp.Core.Unit, Microsoft.FSharp.Collections.FSharpList<FSharp.Compiler.CompilerImports.AvailableImportedAssembly>>>>, FSharp.Compiler.CompilerConfig.ProjectAssemblyDataResult>()
Trampoline.Execute()
[email protected]()
QueueUserWorkItemCallback.WaitCallback_Context()
ExecutionContext.RunInternal()
ExecutionContext.Run()
QueueUserWorkItemCallback.System.Threading.IThreadPoolWorkItem.ExecuteWorkItem()
ThreadPoolWorkQueue.Dispatch()
_ThreadPoolWaitCallback.PerformWaitCallback()
[Native to Managed Transition]
[Application Domain Transition]
[Native to Managed Transition]

@majocha Could that be of interest for you? 🙂
@github-actions github-actions bot added this to the Backlog milestone Jan 14, 2025
@majocha
Copy link
Contributor

majocha commented Jan 14, 2025 •
edited
Loading

Just from reading the code I guess the problem is here

fsharp/src/Compiler/Driver/CompilerImports.fs

Lines 2251 to 2257 in b2f2065

// NOTE: When used in the Language Service this can cause the transitive checking of projects. Hence it must be cancellable.
member tcImports.TryRegisterAndPrepareToImportReferencedDll
(
ctok,
r: AssemblyResolution
) : Async<(_ * (unit -> AvailableImportedAssembly list)) option> =
async {

Despite what the comment says, this is async, not cancellable

To propagate the cancellation token the call to ILModuleDef getter must be wrapped in cancellable somewhere in between. I suspect that's what's causing the problem.

@majocha
Copy link
Contributor

majocha commented Jan 14, 2025

Yes, the thing is, ILModuleReader contract does not suggest cancellability in any way.

fsharp/src/Compiler/AbstractIL/ilread.fsi

Lines 62 to 66 in b2f2065

/// Represents a reader of the metadata of a .NET binary. May also give some values (e.g. IL code) from the PE file
/// if it was provided.
type public ILModuleReader =
abstract ILModuleDef: ILModuleDef
abstract ILAssemblyRefs: ILAssemblyRef list

@auduchinok
Copy link
Member Author

Yes, the thing is, ILModuleReader contract does not suggest cancellability in any way.

Hm, but there are many APIs that aren't themself aware of possible cancellations, and it's fine. What's important is their calls are wrapped in the cancellable computation somewhere.

@majocha
Copy link
Contributor

majocha commented Jan 14, 2025

Hm, but there are many APIs that aren't themself aware of possible cancellations, and it's fine. What's important is their calls are wrapped in the cancellable computation somewhere.

True, but relying on convention over explicit type contract in public api makes me feel uneasy 😄.

I looked through the code and I can see a few other potentially problematic places where ILModuleReader is used with no cancellable in sight.

@auduchinok
Copy link
Member Author

True, but relying on convention over explicit type contract in public api makes me feel uneasy 😄.

True, but I also think we don't want to change the entire IL subsystem to annotate everything as cancellable explicitly.
My rule of thumb would probably be something like "everything during an analysis phase is wrapped into a cancellable computation somewhere" 🙂

@majocha
Copy link
Contributor

majocha commented Jan 14, 2025

True, but relying on convention over explicit type contract in public api makes me feel uneasy 😄.

True, but I also think we don't want to change the entire IL subsystem to annotate everything as cancellable explicitly. My rule of thumb would probably be something like "everything during an analysis phase is wrapped into a cancellable computation somewhere" 🙂

Makes sense :) I'm wondering what to wrap in cancellable to make a reasonably seamless fix.

@T-Gro
Copy link
Member

T-Gro commented Jan 15, 2025

Could we treat everything async around CompilerImports as being a cancellable?
I guess that would require shadowing async with something different, making it somewhat obfuscated to those unaware.. :(

@majocha
Copy link
Contributor

majocha commented Jan 15, 2025

We could maybe use some resumable AsyncEx flavor internally throughout and only keep legacy async on public endpoints.

@majocha majocha linked a pull request Jan 15, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Needs-Triage
Projects
F# Compiler and Tooling
Status: New
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Set Cancellable.token from async computation majocha/fsharp
3 participants
@majocha @auduchinok @T-Gro