Blazor Index Out of Range Exception

[cyberjaws]

Describe the bug

Exception thrown: 'System.IndexOutOfRangeException' in System.Private.CoreLib.dll
System.IndexOutOfRangeException: Index was outside the bounds of the array.
at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)

I have a periodic task that runs at 1Hz and components that get their data and update their state call StateHasChanged() at that frequency.

To Reproduce

1. Using this version of ASP.NET Core: Blazor 0.7.0
2. Run this code:

            _updateTimer = new Timer((state) =>
            {
                try
                {
                    Updated();
                }
                catch(Exception ext)
                {
                    System.Diagnostics.Trace.WriteLine(ext.ToString());
                }
            }, null, TimeSpan.FromSeconds(1), TimeSpan.FromSeconds(1));

3. With these arguments:
   Blazor Components that register for this event call StateHasChanged() when appropriate.
4. See error:

Exception thrown: 'System.IndexOutOfRangeException' in System.Private.CoreLib.dll
System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at Microsoft.AspNetCore.Blazor.Rendering.Renderer.AssignEventHandlerId(RenderTreeFrame& frame)
   at Microsoft.AspNetCore.Blazor.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForAttributeFrame(DiffContext& diffContext, Int32 oldFrameIndex, Int32 newFrameIndex)
   at Microsoft.AspNetCore.Blazor.RenderTree.RenderTreeDiffBuilder.AppendAttributeDiffEntriesForRange(DiffContext& diffContext, Int32 oldStartIndex, Int32 oldEndIndexExcl, Int32 newStartIndex, Int32 newEndIndexExcl)
   at Microsoft.AspNetCore.Blazor.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForFramesWithSameSequence(DiffContext& diffContext, Int32 oldFrameIndex, Int32 newFrameIndex)
   at Microsoft.AspNetCore.Blazor.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForRange(DiffContext& diffContext, Int32 oldStartIndex, Int32 oldEndIndexExcl, Int32 newStartIndex, Int32 newEndIndexExcl)
   at Microsoft.AspNetCore.Blazor.RenderTree.RenderTreeDiffBuilder.ComputeDiff(Renderer renderer, RenderBatchBuilder batchBuilder, Int32 componentId, ArrayRange`1 oldTree, ArrayRange`1 newTree)
   at Microsoft.AspNetCore.Blazor.Rendering.ComponentState.RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment)
   at Microsoft.AspNetCore.Blazor.Rendering.Renderer.RenderInExistingBatch(RenderQueueEntry renderQueueEntry)
   at Microsoft.AspNetCore.Blazor.Rendering.Renderer.ProcessRenderQueue()
   at Microsoft.AspNetCore.Blazor.Rendering.Renderer.AddToRenderQueue(Int32 componentId, RenderFragment renderFragment)
   at Microsoft.AspNetCore.Blazor.Server.Circuits.CircuitSynchronizationContext.ExecuteSynchronously(TaskCompletionSource`1 completion, SendOrPostCallback d, Object state)
   at Microsoft.AspNetCore.Blazor.Server.Circuits.CircuitSynchronizationContext.Post(SendOrPostCallback d, Object state)
   at NG.Blazor.Client.Components.Settings.<OnAfterRender>b__10_0() in C:\compile\BlazorEcdis\NG.Blazor.Client\Components\Settings.cshtml:line 32
   at NG.Blazor.Client.DataService.<.ctor>b__7_0(Object state) in C:\compile\BlazorEcdis\NG.Blazor.Client\DataService.cs:line 31

Expected behavior

Dictionary can be added to.

Screenshots

N/A

Additional context

Likely this could be addressed by changing the Dictionary that houses the _eventBindings (and _componentStateById?) to a Concurrent Dictionary here:
https://github.com/aspnet/AspNetCore/blob/436b5461ad0337aac90089aa7ccb61dd875808e4/src/Components/src/Microsoft.AspNetCore.Components/Rendering/Renderer.cs

[mkArtakMSFT]

Thanks for contacting us, @cyberjaws.
@SteveSandersonMS can you please look into this? Thanks!

[springy76]

This bug still exists in Razor Components 3.0 preview 2 (end of Jan2019).

RemoveEventHandlerIds runs asynchronously (called from thread pool) and corrupts the _eventBindings dictionary by modifiying it without any locking mechanism.

I've seen multiple threads doing RemoveEventHandlerIds in parallel for the very same Renderer instance and also RemoveEventHandlerIds run in parallel with building a new render tree diff.

I'm using no timers but just some OnInitAsync and OnParameterSetAsync overrides .

[DNF-SaS]

Also experiencing this issue here using Razor Components from ASP.NET Core 3.0 Preview 2.

[cyberjaws]

I ported my app to Core 3.0 Preview 2. I was pleased to see cross threading exceptions from calling StateHasChanged() from a Threading.Timer's callback. (They been addressed using the newly supplied Invoke/InvokeAsync methods in the component's base. Thank you for addressing that!) Unfortunately I too still see this bug.

[cyberjaws]

For those tracking this ticket, it looks like some work has been done for the next release that should resolve this issue. Appears that the calls to the Renderer are going to be passed through an Invoker to ensure there won't be cross threaded access to the Dictionaries.

[cyberjaws]

Alas, I am still getting exceptions with Preview 3.

[cyberjaws]

Even with a try catch in Preview 3, the Signal R socket ends up disconnecting when this error occurs.

[JohanDonne]

Having the same problem, including SignalR disconnect. This is pretty disruptive...
edit: using razor components.

[springy76]

While I did not see the IndexOutOfRangeException since preview3, the display freeze followed by a SignalR timeout after 30s in browser still happens far too often.

I made a test project for simulating this: https://github.com/springy76/RazorComponentsProblems/blob/problem/ConnectionDiesWhenUsingAsyncOperations/WebApp/Components/Pages/Index.razor

Sometimes you only have to click some of the 12 buttons, sometimes you have to alternate for a minute. But sooner or later the display will freeze ("loading" text will not disappear, all buttons without function) and 30s later the browser will log connection timeout to debug console.

Even after having disabled "just my code", and enabling every .net exception and every managed debugging assistant in exception settings, I did not see neither an exception in debugger nor deadlocked threads in thread-list. I wonder if anyone can narrow this down?

[cyberjaws]

@springy76 Thanks for posting. These could be related, however I would recommend creating another ticket for the disconnect and then link them.

[springy76]

I changed my sample above and now it crashes always on first click of button 0-4 or 6-11. But only reproducible using a freshly started server. I've seen the exception happen a second time and then even after hitting F5 (which creates a new circuit) it did not happen again.

Stacktrace:

warn: Microsoft.AspNetCore.Components.Browser.Rendering.RemoteRenderer[1]
      Unhandled exception rendering component: Index was outside the bounds of the array.
System.IndexOutOfRangeException: Index was outside the bounds of the array.
   at System.Collections.Generic.Dictionary`2.TryInsert(TKey key, TValue value, InsertionBehavior behavior)
   at System.Collections.Generic.Dictionary`2.Add(TKey key, TValue value)
   at Microsoft.AspNetCore.Components.Rendering.Renderer.AssignEventHandlerId(RenderTreeFrame& frame)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.InitializeNewAttributeFrame(DiffContext& diffContext, RenderTreeFrame& newFrame)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForAttributeFrame(DiffContext& diffContext, Int32 oldFrameIndex, Int32 newFrameIndex)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendAttributeDiffEntriesForRange(DiffContext& diffContext, Int32 oldStartIndex, Int32 oldEndIndexExcl, Int32 newStartIndex, Int32 newEndIndexExcl)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForFramesWithSameSequence(DiffContext& diffContext, Int32 oldFrameIndex, Int32 newFrameIndex)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForRange(DiffContext& diffContext, Int32 oldStartIndex, Int32 oldEndIndexExcl, Int32 newStartIndex, Int32 newEndIndexExcl)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForFramesWithSameSequence(DiffContext& diffContext, Int32 oldFrameIndex, Int32 newFrameIndex)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.AppendDiffEntriesForRange(DiffContext& diffContext, Int32 oldStartIndex, Int32 oldEndIndexExcl, Int32 newStartIndex, Int32 newEndIndexExcl)
   at Microsoft.AspNetCore.Components.RenderTree.RenderTreeDiffBuilder.ComputeDiff(Renderer renderer, RenderBatchBuilder batchBuilder, Int32 componentId, ArrayRange`1 oldTree, ArrayRange`1 newTree)
   at Microsoft.AspNetCore.Components.Rendering.ComponentState.RenderIntoBatch(RenderBatchBuilder batchBuilder, RenderFragment renderFragment)
   at Microsoft.AspNetCore.Components.Rendering.Renderer.RenderInExistingBatch(RenderQueueEntry renderQueueEntry)
   at Microsoft.AspNetCore.Components.Rendering.Renderer.ProcessRenderQueue()
   at Microsoft.AspNetCore.Components.Rendering.Renderer.AddToRenderQueue(Int32 componentId, RenderFragment renderFragment)
   at Microsoft.AspNetCore.Components.ComponentBase.StateHasChanged()
   at WebApp.Components.Pages.Index.SetValueAsync(Int32 newVal) in C:\dev\gitWork\tests\RazorComponentsProblems\WebApp\Components\Pages\Index.razor:line 49
   at Microsoft.AspNetCore.Components.ComponentBase.CallStateHasChangedOnAsyncCompletion(Task task)
   at Microsoft.AspNetCore.Components.Rendering.Renderer.GetErrorHandledTask(Task taskToHandle)

[cyberjaws]

Thanks @springy76. That is the same exception that this thread is tracking. Glad to see that you have code and a process that is repeatable. @SteveSandersonMS do you think this could be addressed before the official release?

[SteveSandersonMS]

I've done some investigation, and now would like to discuss and re-triage. cc @mkArtakMSFT

Although I've been unable to reproduce the IndexOutOfRangeException personally, I think I've found two bugs that are behind this problem.

1. Use of ContinueWith in Renderer.cs

We still have a use of ContinueWith here: https://github.com/aspnet/AspNetCore/blob/master/src/Components/Components/src/Rendering/Renderer.cs#L625

This escapes from the renderer's sync context, leading to general thread-safety issues. In particular, the code that runs here tries to mutate _eventBindings. This is basically what @springy76 was reporting here. Note that the fix is not to use locking, but rather to ensure we stay on the correct sync context so no locking is needed.

2. MessagePack's ReadStringSlow looks broken

Fixing (1) above isn't sufficient. The connection can still become broken, because when there's enough contention, incoming messages get corrupted. What I was seeing was it trying to call a SignalR Hub called OnOnOnOnOnOnOnOnO, when it should have been calling OnRenderCompleted.

I think I've traced this down to a bug around here: https://github.com/aspnet/MessagePack-CSharp/blob/master/src/MessagePack/MessagePackReader.cs#L791

Notice that inside this while loop, it never calls reader.Advance at all. So if the current span is shorter than the length of the entire string, it will just keep copying the contents of the current span over and over until it fills up the correct output length. In my case, the initial span was length 2, so it just keeps duplicating On until it gets to the desired number of characters.

If I modify the code to put this.reader.Advance(bytesRead); right at the end of the while loop, it starts working OK.

Since this is so completely broken and doesn't seem to have been reported yet, it looks like ReadStringSlow is only used incredibly rarely, and you really have to stress the system to get into a scenario where more than one span is involved.

Blazor team: let's discuss how best to get this fix into the MessagePack code.

[springy76]

Would it be possible to resync that ContinueWith code with the correct sync context for preview4 milestone?
(btw: That ContinueWith exactly is what I meant by saying "called from thread pool").

[SteveSandersonMS]

Sorry, no, it's too late.

[springy76]

Thats sad. Since preview1 all of our beta testers get hit by this bug regularily and the only thing they notice is that the app (or sometimes even just a part of it) stops responding to clicks at any random time -- they don't see the exception stacktrace scroll by in console of course.

Shouldn't this be fixable by a "simple" syncContext.Post?