Tests deadlock

[RussKie]

• .NET Core Version: 5.0

Problem description:

Running tests from the master branch locally I got tests hung. I have attached a debugger and there was a dozen of hung threads.

9 threads were stuck unable to acquire the lock in NativeWindow.CreateHandle() method (originating stacks were slightly different, but they all ended in the same critical section):

1 thread was stuck restoring the default trace listener:

2 thread were stuck while writing a trace:

What appears to have happened is that one test was in a middle of creating a handle and attempted to add a trace, which was locked by another thread, that was restoring the default trace listener:

Expected behavior:

No deadlock

Minimal repro:

This is very intermittent...

[RussKie]

Few questions here:

• Do we need to lock in NoAssertContext? ?. operator is considered thread-safe - https://stackoverflow.com/a/35786442/2338036.
  • Can we make s_defaultListener volatile or use Interlocked object?
• Should all out tests dealing with UI (e.g. instantiating controls) be decorated with StaFactAttribute or WinFormsFactAttribute

/cc: @hughbe

[weltkante]

Creating a control handle on a non-STA thread is a bad idea, it turns the thread pool thread into a UI thread by allocating a message queue etc. but without telling COM that the thread is STA, so controls using COM internally (e.g. RichTextBox) will be pretty confused, if working at all.

[JeremyKuhne]

Should all out tests dealing with UI (e.g. instantiating controls) be decorated with StaFactAttribute or WinFormsFactAttribute

I think WinFormsFactAttribute is the right response here

[weltkante]

@JeremyKuhne do you know if WinFormsFactAttribute will lead to OleInitialize being called? I suspect some of the test failures are due to calling OLE functions on non-STA threads, but STA alone may not be enough for OLE, you probably need to call OleInitialize on the thread as well (WinForms does this in most public code paths which e.g. initialize controls, but when doing testing of internal interop it may be circumvented)

[RussKie]

/cc: @AArnott any thoughts on the subject?

[hughbe]

Could it be that we don’t dispose any controls in the unit tests?

[weltkante]

I think there are multiple concerns here

The deadlock @RussKie shows is probably just a bug in the NoAssertListener, the TraceListenerCollection has its own lock. I didn't do a full analysis but it looks like you got a lock inversion with the NoAssertListeners lock: one path takes them in order A B the other code path takes them in order B A, leading to a deadlock.

Then there is the whole concern of running tests involving window handles and OLE interop on non-STA threads, which technically is not part of this issue, I just was asking here because people already were discussing the test attributes after my first comment.

Not disposing a control may be a problem, but its not directly related to the deadlock. My first comment about control handle creation was just a general comment without having looked at the problem in detail, sorry for leading the discussion astray.

I still think the whole STA/OLE initialization needs to be examined though, maybe in context of some of the other issues about unstable tests.

[AArnott]

The WinFormsFactAttribute executes the test on an STA thread and sets up WinFormsSynchronizationContext on that thread. It does nothing else. If OleInitialize or some other code should be run as well before the test starts, I'm happy to oblige. Please file an issue at https://github.com/AArnott/Xunit.StaFact/issues