Closed cyberjaws closed 5 years ago
Thanks for contacting us, @cyberjaws. @SteveSandersonMS can you please look into this? Thanks!
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 .
Also experiencing this issue here using Razor Components from ASP.NET Core 3.0 Preview 2.
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.
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.
Alas, I am still getting exceptions with Preview 3.
Even with a try catch in Preview 3, the Signal R socket ends up disconnecting when this error occurs.
Having the same problem, including SignalR disconnect. This is pretty disruptive...
edit: using razor components.
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?
@springy76 Thanks for posting. These could be related, however I would recommend creating another ticket for the disconnect and then link them.
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)
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?
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.
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.
ReadStringSlow
looks brokenFixing (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.
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").
Sorry, no, it's too late.
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
?
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
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