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.

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

m_UnresolvedClassLock is held in an unsafe way for Func Evals #60565

Closed
auott opened this issue Oct 18, 2021 · 24 comments · Fixed by #72179
Closed

m_UnresolvedClassLock is held in an unsafe way for Func Evals #60565

auott opened this issue Oct 18, 2021 · 24 comments · Fixed by #72179
Assignees
Milestone

Comments

@auott
Copy link

auott commented Oct 18, 2021

Description

m_UnresolvedClassLock can be held when an application is in break state. If a func eval triggers an assembly load while that lock is held on another thread the func eval must be aborted.

Reproduction Steps

See the following VS feedback ticket: https://developercommunity.visualstudio.com/t/VS2022-preview-41---debugger-message-T/1548980.

Expected behavior

A func eval should be able to load an assembly without encountering the effective deadlock with m_unresolvedClassLock.

Actual behavior

Func evals can effectively deadlock and the application ends up in a bad state.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Oct 18, 2021
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@stephentoub
Copy link
Member

stephentoub commented Oct 18, 2021

What is "m_unresolvedClassLock"? Can you point to the code you're referring to in the repo? I don't see that name anywhere in dotnet/runtime.

EDIT: I see, casing was wrong.

@auott
Copy link
Author

auott commented Oct 18, 2021

@auott auott changed the title m_unresolvedClassLock is held in an unsafe way for Func Evals m_UnresolvedClassLock is held in an unsafe way for Func Evals Oct 18, 2021
@ghost
Copy link

ghost commented Oct 18, 2021

Tagging subscribers to this area: @vitek-karas, @agocke, @VSadov
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

m_UnresolvedClassLock can be held when an application is in break state. If a func eval triggers an assembly load while that lock is held on another thread the func eval must be aborted.

Reproduction Steps

See the following VS feedback ticket: https://developercommunity.visualstudio.com/t/VS2022-preview-41---debugger-message-T/1548980.

Expected behavior

A func eval should be able to load an assembly without encountering the effective deadlock with m_unresolvedClassLock.

Actual behavior

Func evals can effectively deadlock and the application ends up in a bad state.

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: auott
Assignees: -
Labels:

area-AssemblyLoader-coreclr, untriaged

Milestone: -

@hoyosjs hoyosjs added area-Diagnostics-coreclr and removed area-AssemblyLoader-coreclr untriaged New issue has not been triaged by the area owner labels Oct 18, 2021
@tommcdon tommcdon added this to the 7.0.0 milestone Oct 18, 2021
@auott
Copy link
Author

auott commented Oct 25, 2021

This same issue locking issue also looks like it applies to "m_PinnedHeapHandleTableCrst" in "BaseDomain::AllocateObjRefPtrsInLargeTable".
This was reported via another VS feedback ticket: https://developercommunity.visualstudio.com/t/step-into-while-debugging-intermittent-hangwarning/1561972

@thejman1989
Copy link

thejman1989 commented Dec 9, 2021

Is there any idea on a fix? As this has been stale for some time now. I am also experiencing this error and it's making debugging quite a pain.

@schwarzr
Copy link

Looks like this is related to the hot reload feature. For me the error disappeared after disabling hot reload...

@NoahStahl
Copy link

Same, this behavior ceases for me when hot reload is disabled. Nice find @schwarzr ! Pretty consistent when enabled, even in simple .NET 6 console app. Using latest VS 2022.

@sec
Copy link
Contributor

sec commented Dec 21, 2021

Also getting this - was this fixed in 6.0.1 by any chance? This bug makes Hot Reload (and also Edit and Continue) pretty useless (or annoying at least)...

@EgorBo
Copy link
Member

EgorBo commented Jan 2, 2022

I hit it quite often 😢

@deeprobin
Copy link
Contributor

I hit it quite often 😢

Unfortunately, I have also had this experience 😢.

@EgorBo
Copy link
Member

EgorBo commented Jan 2, 2022

If JetBrains Resharper is installed - it tries to show all locals automatically inside the editor inline during debug break, so this issue is reproduced with every second run for me, when I disabled that feature in Resharper - it gone away.

@thejman1989
Copy link

For me it happens without ReSharper installed. I'm happy it resolved the issue for you, but I'm still experiencing it, which made me switch back to .Net 5

@alexhelms
Copy link

Any update on this? I've been running into this constantly. If I turn off hot reload I no longer have the issue but hot reload/edit and continue are pretty essential.

@JKamsker
Copy link

JKamsker commented Apr 5, 2022

Any update on this? I've been running into this constantly. If I turn off hot reload I no longer have the issue but hot reload/edit and continue are pretty essential.

Same here. Resharper isnt installed aswell.

@marxxxx
Copy link

marxxxx commented May 2, 2022

Same here. This is quite annoying. I turned off hot reload and Edit&Continue as this feature wasn't working properly (at least with Blazor Server) anyway.

@danmoseley
Copy link
Member

@noahfalk to see updates above.

@noahfalk
Copy link
Member

noahfalk commented May 2, 2022

@tommcdon @davmason

@nicklasjepsen
Copy link

Console app, debug mode with break point. Get the error on the first line of execution after stepping over (F10). Disabling "Edit and continue hot reload" makes the error go away.

@tommcdon
Copy link
Member

Thanks for sharing that this issue reproduces with console apps and a workaround @nicklasjepsen. This should help us construct a repro to help us understand and develop potential fixes for it.

@noahfalk noahfalk assigned noahfalk and unassigned davmason Jul 8, 2022
@noahfalk
Copy link
Member

noahfalk commented Jul 8, 2022

Some research into the m_UnresolvedClassLock suggests that this specific cause of the issue was probably resolved when this code was changed early in .NET 7. This doesn't guarantee that all causes of the issue are resolved, or that everyone observing that func-eval abort message was hitting this specific cause, but it should at least mean the situation is improved in .NET 7. Has anyone reproducing the issue on .NET 6 been able to try a .NET 7 preview build to see if the issue is resolved for you? If you can still reproduce it on a .NET 7 preview I'd love to get some dumps from you to try and diagnose any additional variations of this problem.

I tried reproducing the problem as described by @nicklasjepsen but I wasn't able to observe any issue. Here is what I did specifically in case anyone has an alternate suggestion of what I should try?

  1. Launch Visual Studio 2022 (v17.2.5)
  2. Create a new Console app project targetting NET 6.0 using the default top level statements. This creates the standard two line Program.cs:
// See https://aka.ms/new-console-template for more information
Console.WriteLine("Hello, World!");
  1. I set a breakpoint on line 2
  2. I hit F5 with the hot reload settings still in their default on configuration and the project in the default Debug configuration
  3. After hitting the breakpoint at line 2 I used F10 to step over
    The step completed as expected, no errors shown.

I also tried variations where I made edits before hitting F10, did multiple steps, used set next statement, and I changed the project from console app to asp.net core app. None of those variations produced any errors for me.

This same issue locking issue also looks like it applies to "m_PinnedHeapHandleTableCrst" in "BaseDomain::AllocateObjRefPtrsInLargeTable".

@auott - in order for someone on the runtime to investigate we either need callstacks where you observed the threads blocked or dumps from debuggee. Do you have them?

@noahfalk
Copy link
Member

noahfalk commented Jul 8, 2022

@auott - actually looking at m_PinnedHeapHandleTableCrst this is probably simple enough that I can guess what the repro is even if you don't have stacks. They'd still be handy if you have them though.

@auott
Copy link
Author

auott commented Jul 8, 2022

@noahfalk - I don't have any stacks at this point. I also haven't seen any repros that have come from .NET 7 (at least for this specific one) so it's definitely possible some of this has been cleaned up.

noahfalk added a commit to noahfalk/runtime that referenced this issue Jul 14, 2022
Fixes dotnet#60565

Visual Studio devs reported that debugger funcevals were deadlocking because the
PinnedHeapHandleTableCrst was held while threads were suspended. This change
refactors that code path so that the AllocateHandles() operation where the lock is held
gets split into potentially multiple iterations and the GC allocation where the debugger
could suspend is extracted outside the region where the critical section is held. There is
another path into AllocateHandles that takes a different lock and theoretically it could
generate a similar deadlock, but I didn't do all the refactoring necessary to prevent
that path.
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2022
@noahfalk
Copy link
Member

I just put up a PR that should resolve the PinnedHeapHandleTableCrst part of the issue. With no known repro I can't guarantee it fixes the problem, but I think I have a decent guess what happened and I am able to use the debugger to confirm that the new debugger suspension point is outside the PinnedHeapHandleTableCrst lock.

I am planning to mark the bug resolved once that fix gets checked in and if there are new func-eval deadlocks we can file those as new issues and evaluate them case by case. The runtime has never provided a guarantee that it is safe to run arbitrary func-evals when stopped at arbitrary places, but pragmatically we'll try to resolve any common obstacles if we can.

noahfalk added a commit that referenced this issue Jul 22, 2022
Fixes #60565

Visual Studio devs reported that debugger funcevals were deadlocking because the
PinnedHeapHandleTableCrst was held while threads were suspended. This change
refactors that code path so that the AllocateHandles() operation where the lock is held
gets split into two parts and the GC allocation where the debugger could suspend is
outside the region where the critical section is held. 

In the old code the PinnedHeapHandleTable was synchronized by one of two different locks, either the AppDomainHandleTable lock or the GlobalStringLiteralMap. In the new code AppDomainHandleTable lock is renamed to PinnedHeapHandleTable lock and this lock is always what synchronizes the PinnedHeapHandleTable code. In the string literal code path the GlobalStringLiteralMap is taken first and the PinnedHeapHandleTable lock is taken second, but the PinnedHeapHandleTable is no longer reliant on that outer GlobalStringLiteralMap lock for correctness.

In terms of testing I can verify under a debugger that I can suspend in the GC allocation point and the PinnedHeapHandleTable lock isn't held. This doesn't of course prevent other locks from being held so at best it is a partial fix for the issue. Nobody had a known repro so I wasn't able to verify anything more specifically. I also confirmed the race cases on the InterlockedExchange paths worked how they were intended by forcing them with a native debugger.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 22, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 22, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.