Fix NullReferenceExceptions caused by WorkItemQueue not being thread-safe

[ChrisMaddock]

NUnit 3.8 started throwing various null reference exceptions e.g.

 Unhandled Exception: System.NullReferenceException: Object reference not set to an instance of an object.
    at NUnit.Framework.Internal.Execution.WorkItemQueue.Dequeue()
    at NUnit.Framework.Internal.Execution.TestWorker.TestWorkerThreadProc()
    at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
    at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
    at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
    at **System.Threading.ThreadHelper.ThreadStart()**

at NUnit.Framework.Internal.Execution.WorkItemQueue.get_IsEmpty()
at NUnit.Framework.Internal.Execution.WorkShift.get_HasWork()
at NUnit.Framework.Internal.Execution.ParallelWorkItemDispatcher.StartNextShift()
at NUnit.Framework.Internal.Execution.ParallelWorkItemDispatcher.OnEndOfShift(Object sender, EventArgs ea)
at NUnit.Framework.Internal.Execution.WorkShift.<StartWorkers>b__26_1(TestWorker s, WorkItem ea)
at NUnit.Framework.Internal.Execution.TestWorker.TestWorkerThreadProc()
at System.Threading.ExecutionContext.RunInternal(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state, Boolean preserveSyncCtx)
at System.Threading.ExecutionContext.Run(ExecutionContext executionContext, ContextCallback callback, Object state)
at System.Threading.ThreadHelper.ThreadStart()

@garuma diagnosed the below

This seems to come from the queue array having null in it. In turn, I believe this is due to InitializeQueues being not concurrent-safe, creating an array and initializing it directly on shared state.

In #2417 @garuma is proposing a general refactor of this section of code. @CharliePoole suggested that we should put in a bare minimum fix as a hotfix, and do the refactor separately.

[rprouse]

Over the past couple of weeks, I have been running and re-running a series of integration style tests for our numerous parallization tests after the recent fixes and I have not seen this NullReferenceException since those fixes went in although I did see it when I was reproducing the fixes with 3.8.1.

I think this is fixed, but I am going to leave open to track while I finish testing the remaining parallel issues. If this does not come up again, I think we might be able to close. What do you think @ChrisMaddock ?

[ChrisMaddock]

Sorry @rprouse - this one got lost.

I think this code has changed sufficiently that the original issue we found would no longer be applicable. I haven't reviewed the current code enough to check that - @CharliePoole may have an answer off the top of his head though. 🙂

[CharliePoole]

If the nulls were caused by concurrency issues, I'd expect we have either cured the problem or at least cut down on it's frequency.

[garuma]

If someone can point me at a commit that's expected to work I can try to run against our suite again and see if I can still see the NREs