Avoid clearing thread local handles for already unloaded loader #99998
Conversation
(cherry picked from commit 269c6fb)
dotnet-issue-labeler
bot
added
the
needs-area-label
dotnet-policy-service
bot
added
the
community-contribution
|
@dotnet-policy-service agree company="Unity Technologies" |
jkotas
added
area-VM-coreclr
and removed
needs-area-label
| } | ||
| if (entry->m_hNonGCStatics != NULL) | ||
| // Skip assemblies that are already unloaded | ||
| if (!pLoaderAllocator->IsUnloaded()) |
There was a problem hiding this comment.
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.
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. |
Motivation:
Fix crash in
ThreadLocalBlock::FreeTLMcode on thread exit which happens during Assembly unloading when Assembly contains a class with aThreadStaticvariable.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:
loaderAllocatorvalue 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_LOADSCLR 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:
loaderAllocatorI think skipping unloaded loader is the best option wrt performance/correctness considerations as loader deletion will cleanup all internal data, we won't access memory which may be already returned to pool/system and we won't lock other threads.