Workaround a CLR deadlock caused by unloading app domains with STA RCWs #34498
Conversation
jasonmalinowski
force-pushed
the
work-around-appdomain-rcw-deadlock
branch
from
613d56f to
0d84adc
Compare
jasonmalinowski
force-pushed
the
work-around-appdomain-rcw-deadlock
branch
from
0d84adc to
0330bf6
Compare
|
At this rate I really hate to imagine what the third hack in this block will look like |
| // | ||
| // The suggested workaround from the CLR team is to do an explicit GC.Collect and | ||
| // WaitForPendingFinalizers before we let the AppDomain shut down. The belief is subscribing | ||
| // to the Unload is a reasonable place to do it. |
There was a problem hiding this comment.
❓ Are we sure the RCWs will get cleaned up before the STA thread is terminated?
There was a problem hiding this comment.
So the STA thread tries to clean up it's own RCWs on shutdown of the thread -- in the dumps I saw that lead to the internal conversation it was that process that was waiting for finalization (and finalization on the thread).
There's no guarantee from the CLR team that this will guarantee RCW cleanup, but it seemed that there's really no way to make the guarantee short of the CLR making some pretty deep changes to how this all works internally, for example by changing how AppDomain Unload works internally to correctly order operations.
| // to the Unload is a reasonable place to do it. | ||
| if (Interlocked.CompareExchange(ref s_SubscribedToAppDomainUnload, 1, 0) == 0) | ||
| { | ||
| AppDomain.CurrentDomain.DomainUnload += (sender, e) => |
There was a problem hiding this comment.
Does this event happen before or after Thread.Abort is called on our STA thread?
There was a problem hiding this comment.
From the mail thread with the CLR team, before.
There was a problem hiding this comment.
Hmm ... if that's the case I wonder if we should call Dispatcher.Shutdown here as well. That would seemingly solve the unload event race that's displayed below. That's a separate issue though.
In reply to: 269633055 [](ancestors = 269633055)
There was a problem hiding this comment.
That would work. I guess there's also good arguments here that we should just be making this thread a test fixture so it's properly shut down when we're done using it.
There was a problem hiding this comment.
I looked into that a bit. The setup works and we have a definite time in which to do shutdown operations. At the same time though:
- It still requires passing
staticdata. There is no way for the test fixture and test runner to communicate via the xunit APIs. That means mutablestaticis the only way. - It requires a lot of source annotations. Including duplicating definitions between all our test assemblies. That's annoying but not a deal breaker.
That's why I haven't moved forward on it.
There was a problem hiding this comment.
Ah, right forgot about 1. I think the "usual" asumption is instead we'd switch our tests to regular Facts and just have all our Wpf ones do something like _staThread.RunOnThread() and then pass the lambda of the actual test contents. Or do an await SwitchToStaThread() pattern not unlike JTF.
| { | ||
| AppDomain.CurrentDomain.DomainUnload += (sender, e) => | ||
| { | ||
| GC.Collect(); |
There was a problem hiding this comment.
Most of the time I see the Collect + WaitForPendingFinalizers pattern it's done in a loop. Assume there is a reason you didn't need to do that here. Curious about what it would be.
There was a problem hiding this comment.
I wasn't sure how much to loop. So, ¯\_(ツ)_/¯?
There was a problem hiding this comment.
The magic number I'm thinking of here is three. I have no idea why I I think that though.
There was a problem hiding this comment.
Tradition dictates three, but that's cargo cult programming if I've ever seen it.
There was a problem hiding this comment.
jasonmalinowski
force-pushed
the
work-around-appdomain-rcw-deadlock
branch
2 times, most recently
from
58f0b4e to
3a190d7
Compare
If you have an STA thread that created Runtime Callable Wrappers, it's possible to end up in a deadlock where the finalizer and app domain unload code are waiting for each other. The suggested workaround from the CLR team is to wait for finalizers before letting the domain shutdown but while the thread is still running. Fixes dotnet#34248
jasonmalinowski
force-pushed
the
work-around-appdomain-rcw-deadlock
branch
from
3a190d7 to
bea2405
Compare
| AppDomain.CurrentDomain.DomainUnload += (sender, e) => | ||
| { | ||
| GC.Collect(); | ||
| GC.WaitForPendingFinalizers(); |
There was a problem hiding this comment.
You've sent a PR that uses a static consturctor to hook AppDomain.Unload so we can call GC.Collect and I can't argue with it because it's probably the right thing to do.
If you have an STA thread that created Runtime Callable Wrappers, it's possible to end up in a deadlock where the finalizer and app domain unload code are waiting for each other. The suggested workaround from the CLR team is to wait for finalizers before letting the domain shutdown but while the thread is still running.
Fixes #34248