Concurrency issues around dispatch events and waiting for renders

[egil]

The following error shows up in 154 test runs out of about 3000:

• Htmlizer.cs:line 44, Assert.Debug (20 times)
• Htmlizer.cs:line 166, Assert.Debug (14 times)
• Htmlizer.cs:line 59, error InvalidOperationException : We didn't consume any input (33 times)
• Htmlizer.cs:line 93 , error InvalidOperationException : Invalid element frame type 'None' (28 times)
• The HashSet Observers in RenderEventObservable.cs is updated concurrently. Throws the following error message: Operations that change non-concurrent collections must have exclusive access. A concurrent update was performed on this collection and corrupted its state. The collection's state is no longer correct. (3 times)
• Additional regular assertion fails, probably just related to the above. Many timeouts.

[egil]

Current thinking is that the issue is that subscribers to render events are in the processes of regenerating their DOM while the next render happens, since they are waiting on one thread and the rendere continues its work on another thread. Thus the OnRender trigger isn't actually synchronous and blocking until the DOMs are updated, it just blocks until the waiting threads are notified, and they go off and do their own work.

Next thing to attempt is to change the notification system such that subscribers can return a Task when notified, that they should complete when they are done doing whatever work they need to do. Looking at the Renderer.cs code, it seems like it will await an uncompleted task before continuing with the next render - https://source.dot.net/#Microsoft.AspNetCore.Components/RenderTree/Renderer.cs,514

[egil]

Pushed a new branch with semi complete attempt at new render event notification system. Needs more testing.

Also, this cloud enable making tasks returned by e.g. ClickAsync first complete after the new DOM elements have been generated, making the use of the WaitFor* methods less needed... Further investigation required!

[egil]

Second update with a complete new notification pipeline, that looks like this:

After a render is complete, but before ComponentBase.OnAfterRender* methods are called, this happens:

1. TestRenderer.UpdateDisplayAsync is invoked, it calls to Task RenderedFragment.Handle()
2. Task RenderedFragment.Handle() checks if it or its children are apart of the render, if they are, it notifies subscribers to the event RenderedFragment.OnAfterRender.
3. Then it checks if there are any markup changes in it or its children. If there are, it updates the Markup property and nulls the Nodes property, and then notifies subscribers of RenderedFragment.OnMarkupUpdated.
4. Then it completes by returning a completed task, which the renderer is awaiting.

The core of the problem seems to have been:

1. The order when the "wait for's" was notified of changes did not necessarily occur after the rendered fragments was notified and had markup and anglesharp nodes regenerated, which caused the state predicate or assertion to fail when it didn't have to.
2. The thread that runs the wait for checker and the other thread that runs the renderer, could be running on different CPUs/cores, and that would cause memory coherence problems, where the update to markup and nodes, even if they happened before the wait for checks, would not be seen by the checker, and still fail.
3. There was a race condition between nulling out the nodes property and generating new markup, that could lead to the wait for checkers to get an older version of the nodes even though the markup was correctly updated.

To solution seems to be:

1. In the UpdateDisplayAsync in the test renderer, return a task that does not complete before all the listening rendered fragments have regenerated their markup and nodes, and all their wait for listeners have executed their logic. The incomplete task looks like it causes the renderer to wait before any OnAfterRender* methods are called.
2. Use Volatile.Write/Volatile.Read when accessing RenderedFragment.Nodes and RenderedFragment.Markup, to ensure that memory coherence is not a problem.
3. Synchronize access to RenderedFragment.Nodes and RenderedFragment.Markup via the lock statement, both when updating and reading, to avoid the two getting out of sync, e.g. nodes representing an older version of the the current markup.
4. Adding locking in the wait for helpers around the checking and timeout, to avoid race conditions in them, e.g. if the timer invokes the OnTimeout method while the OnAfterRender method is going. The lock is around the TaskCompletionSource, to ensure that there is not a race to return a value.

I am not convinced that the problem is gone. For example, if something inside a component under test, e.g. a public property, that the user is checking is changed in the same manner that can cause problems with the rendered markup, then that might suffer from the same problem. I did write a test very similar the markup ones that used to fail. It didn't fail during an overnight run, so there might be some mitigating factors in play.

[egil]

This blog describes the memory coherence problem Ive observed: http://benbowen.blog/post/cmmics_iii/

[egil]

@duracellko, you might find this issue interesting.

The relevant files to look into is:

• https://github.com/egil/bunit/blob/dev/src/bunit.core/Rendering/TestRenderer.cs#L141
• https://github.com/egil/bunit/blob/dev/src/bunit.web/Rendering/RenderedFragment.cs#L138
• https://github.com/egil/bunit/blob/dev/src/bunit.core/Extensions/WaitForHelpers/RenderedFragmentWaitForHelperExtensions.cs
• https://github.com/egil/bunit/blob/dev/src/bunit.core/Extensions/WaitForHelpers/WaitForHelper.cs
• https://github.com/egil/bunit/blob/dev/src/bunit.core/Extensions/WaitForHelpers/WaitForAssertionHelper.cs
• https://github.com/egil/bunit/blob/dev/src/bunit.core/Extensions/WaitForHelpers/WaitForStateHelper.cs

[duracellko]

@duracellko, you might find this issue interesting.

Is there a branch with failing tests? Are those files already fixing the issue?

Btw. I noticed in RenderedFragment

lock (_lock)
{
    return Volatile.Read(ref _markup);
}

If locking is consistent (applied to all access), then Volatile.Read/Write is not necessary.