-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Avoid clearing thread local handles for already unloaded loader #99998
Avoid clearing thread local handles for already unloaded loader #99998
Conversation
@dotnet-policy-service agree company="Unity Technologies" |
src/coreclr/vm/threadstatics.cpp
Outdated
} | ||
if (entry->m_hNonGCStatics != NULL) | ||
// Skip assemblies that are already unloaded | ||
if (!pLoaderAllocator->IsUnloaded()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we run into a situation where the thread gets rescheduled right after this check, the code in GCLoaderAllocators sets the flag and start destroying the loader allocators, and then the thread running this code gets scheduled back and still hits this crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good question! I think it may happen and ideally FreeHandle
call of LoaderAllocator itself should deal with its own state then, perhaps we could use m_crstLoaderAllocator
lock, but it would mean some perf hit for the fast pointer path
my ideal expectation would be that since DeleteThreadStaticData
is under GCX_COOP
in Thread::OnThreadTerminate
that should hopefully mean that once we are in DeleteThreadStaticData
call we know for sure whether or not LoaderAllocator
is alive or not and that state can not change - perhaps IsAlive()
should be used as IsUnloaded
is set on finalizer thread and not relevant to GCX_COOP
🤔
I'm in the process of trying to redo how all thread statics work at the moment, could we hold off on this change for a week or two while I see if I can get my larger change checked in. As it happens I had already identified the problem and the general gist of the solution I came up with is the same, although the mechanism is a fair bit different. |
We (Unity) can confirm this fix is not enough and the race is still here: |
@davidwrighton 's fix is included in #99183.
#99183 is a large refactoring, it is not back portable. The fix for .NET 8 servicing would have to be a small, targeted fix. |
@davidwrighton @jkotas do you think using the idea there is that if exposed object is alive we can safely free LoaderAllocator handles and while we are doing that it won't be collected and thus finalized |
@alexey-zakharov Yes, that looks pretty reasonable, and should fix the issue. If you create a PR against current runtime, I'd approve that change, and then we can port it to .NET 8. If you can do that soonish, you'll likely beat my larger change in, as that's proving harder to finalize than I hoped. |
…collectible LoaderAllocators
b89d190
to
fff7409
Compare
thanks for the update @davidwrighton !
the fix seemed to be running fine for us - no crashes or deadlocks occurred since we've incorporated it. |
@davidwrighton thanks for taking the fix in!
to clarify - should I make a similar PR against .NET 8 branch? |
Number of tests hit failures after this change #102719 . I am sorry I have to revert it to keep the CI clean. Could you please look into fixing this failure and resubmit the change? We should make sure to trigger outer loop tests on the resubmitted PR before it gets merged. |
thanks for the ping @jkotas ! I've reverted the unnecessary constraint changes which caused issues and running tests in #102797, let me know if there is a specific test I can run to validate potential regressions |
…collectible LoaderAllocators (dotnet#99998)
Motivation:
Fix crash in
ThreadLocalBlock::FreeTLM
code on thread exit which happens during Assembly unloading when Assembly contains a class with aThreadStatic
variable.Details:
There seem to be a race condition between LoaderAllocator cleanup during garbage collection and thread locals cleanup on thread exit. Racing stacks are the following:
ALC/LoaderAllocator cleanup
Thread exit with threadlocal cleanup
with an exception which is thrown with the following details:
loaderAllocator
value from theLOADERALLOCATORREF loaderAllocator = (LOADERALLOCATORREF)ObjectFromHandle(m_hLoaderAllocatorObjectHandle);
call is NULL which makes sense, because finalizer thread just disposed data for some of the same LoaderAllocator assemblies.Note: enabling
COR_PRF_MONITOR_MODULE_LOADS
CLR profiler flag increases chances for the race condition as it slows down a bit complete loader destruction.Changes:
I've looked at the following options as a solution to the problem:
loaderAllocator
Based on the discussion the acceptable solution would be to: