aspnetcore: Add StateHasChanged(async: true) that guarantees never to run synchronously

@danroth27 In Gitter, you asked us to flag you on important issues that need attention. This is one of them. Any clues as to its position on the roadmap would be huge, thank you!

Hi @SteveSandersonMS

I too consider it a bug, simply because it is inconsistent. I think it should work the same whether it’s before or after an await, regardless of which approach is decided upon (multiple renders or a single render).

At the moment it is inconsistent and unexpected. It’s taken a long time for people to notice, and it will definitely trip people up.

I like the idea of having another method. But I think instead of a parameter named async perhaps it should be something like bool forceImmediateRender?

Maybe mark StateHasChange() as [Obsolete] so that people will instead call StateHasChanged(bool forceImmediateRender). Over time you can remove the parameterless version and then default forceImmediateRender to false?

For example, if your JS-side code expects the DOM to be updated in response to rendering, you’d have no way to do that if rendering was delayed by an unspecified asynchronous period.

Could you have a task that is completed once the render happens, and any calls marshalled from JS->C# will await that task before completing the JS promise and letting the JS code continue?

Same for unit testing, if it could access the queue then it too could await the task?

PS: Thanks for the interesting discussion 👍

@SQL-MisterMagoo from your example above, I think you found a simpler repro case of the bug. No Javascript involved.

    private async void UpdateComponent()
    {
        System.Diagnostics.Debug.WriteLine("Before First Update");
        component1.UpdateTheComponent();
        System.Diagnostics.Debug.WriteLine("After First Update");
        await Task.Delay(1000);
        System.Diagnostics.Debug.WriteLine("Before Second Update");
        component1.UpdateTheComponent();
        System.Diagnostics.Debug.WriteLine("After Second Update");
    }

In the code above the are 2 identical calls to the method UpdateTheComponent(). The first call is correctly executed and the second shows the bug. Lets see the log

Before First Update After First Update ComponentRendered Before Second Update ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered ComponentRendered After Second Update

When executing the first time, the StateHasChanged correctly flag the component and the render is correctly executed when the thread is released (because of the await Task.Delay). The second time the StateHasChanged executes the render directly which is the bug. The StateHasChanged should never execute the render directly, it should always flag it and queue it so the render is executed when the render thread is free.

In case anyone wants to justify this is by design, notice that component developers can’t expose api that reliably render the component without causing incomprehensible performance degradation. We should document in the api of every method that it must be called before any “await” otherwise the performance will suffer a bunch of repeated rendering. It is a complete nonsense. The platform must be reliable. This is a huge bug that must be fixed soon.

@arivoir Thanks for the additional information.

I see this is inconvenient in your case.

Again, the StateHasChanged should never call a render.

Overall I think there’s a mismatch of expectations here. You want StateHasChanged to do something different from what it does in current releases. For back-compatibility reasons, I don’t think we would change the meaning of StateHasChanged.

Would it be fair to say that you are looking for some new API which means “schedule a render asynchronously but definitely don’t do it right now”? If so that’s a totally reasonable request, and we could definitely look at adding that in the future. It might look like this:

StateHasChanged(async: true);

We could add a feature like that, but we wouldn’t take away the ability to trigger a synchronous render, as that is needed in cases like unit testing or certain JS interop scenarios.

In the meantime, component/app developers can work around it by adding that sort of API themselves. For example, on your component base class, add a method like this:

TaskCompletionSource<bool> scheduledRenderTcs;

protected async Task StateHasChangedAsync()
{
    if (scheduledRenderTcs == null)
    {
        // No render is scheduled, so schedule one now
        var tcs = scheduledRenderTcs = new TaskCompletionSource<bool>();
        await Task.Yield();
        StateHasChanged();
        scheduledRenderTcs = null;
        tcs.SetResult(true);
    }
    else
    {
        // Just return the task corresponding to the existing scheduled render
        await scheduledRenderTcs.Task;
    }
}

This is a bit of a fancy way of doing it (simpler ways are possible), but this way gives you back a Task that you can optionally await on the caller side if you want to know when it’s actually scheduled the render.

So with this, you can replace calls to StateHasChanged with StateHasChangedAsync and get the behavior you want, I hope!

Does that sound reasonable? I know you want the default behavior to change, but like mentioned above, that’s almost certainly not something we’d do because of the back-compatibility issues and the fact that we do want there to be a way of starting the render batch synchronously.

We can look into adding a StateHasChanged(async: true) method in the future if there’s community demand for it.

@arivoir I go back to my previous question. Why are you calling StateHasChanged multiple times? Why not call it when you want a render? What is the situation where a call from JS requires you to call StateHasChanged repeatedly?

I answered that above.

For example, if a complex UI subscribes to 10 pieces of state and all 10 update then it will cause 10 renders.

Calling StateHasChanged in a loop isn’t the goal, it just demonstrates that the component is being rendered immediately rather than simply being flagged.

For example, if a single operation causes multiple changes to shared state that a component is subscribed to then it will receive multiple notifications to rerender itself.

@arivoir I agree. I initially misunderstood your example and posted an erroneous response. Upon realising my error I deleted the comment 😃

The renderer dispatches the Blazor click event through Renderer.DispatchAsync, which sets _isBatchInProgress = true;

https://github.com/dotnet/aspnetcore/blob/c74e9efd0e6ea5b4d6737d23b92042772a50a249/src/Components/Components/src/RenderTree/Renderer.cs#L213-L256

This makes the render requests batch all 100 calls immediately.

Howevever, the codepath for JSInvokable does not go through Renderer.DispatchAsync, which makes the renderer set _isBatchInProgress only during the actual the actual processing of the batch, which is a direct result of the StateHasChanged() call. So the render calls are handled for every StateHasChanged() until MaxBufferedUnacknowledgedRenderBatches is hit in the RemoteRenderer. That’s after 10 render calls. The rest is simply dropped. I’m not sure whether this is intended behaviour and you just should not call StateHasChanged multiple times during JSInvoke, or that it’s a bug.

@SQL-MisterMagoo, all your reasoning is based on a mistake, it isn’t calling the StateHasChanged in an asynchronous method. It is synchronous. It’s a synchronous for, there is no await, therefore it shouldn’t execute the rendering multiple times. It should flag it and execute it just one time. Other behavior not obeying this is a major design failure in the platform and should be the priority 1 of the team because this affects the performance tremendously.

When developing any component with a relative degree of complexity, the ui can be invalidated for diverse causes, coming from different parts of the code, if the component can’t rely in the platform to invalidate the ui correctly without causing a massive numbers of renderings, the component itself needs to do it, which is frankly inviable.

Well, I really wasn’t expecting that. Good find!

FYI, I just ran into a situation where calling StateHasChanged() caused some code to soft-deadlock (oddly, not there, but later on), and replacing it with await StateHasChangedAsync() produced the desired effect both before and after. It’s a complex mesh of third-party libraries and JS interop, so I don’t have a concise analysis of where or why it got stuck. But this is perhaps indicative that this would be helpful to provide by default.

I wrote this so my component wouldn’t update more than X times per second…

https://github.com/mrpmorris/Fluxor/blob/master/Source/Fluxor/UnsupportedClasses/ThrottledInvoker.cs

Actually, what would be really nice would be if there were two options in the parameters 1: Queue or RenderImmediately 2: Ignore or honour ShouldRender (Default / Forced)

That way when using something like Reactive patterns we could have a base component that always returns false from ShouldRender but then when a reactive subscription fires the base component does a Queue + Forced render. That way we get optimised rendering (Queue) and only rendering when the state changes via RX.

@SteveSandersonMS The StateHasChangedAsync workaround looks working, for some cases at least. Thanks for it.

we do want there to be a way of starting the render batch synchronously.

How? The StateHasChanged is not reliable to perform a render synchronously. Is there more api to reliably perform a render?

IMO, the current StateHasChanged implementation is a bug. It’s inconceivable one method does one thing if you execute it either before or after an await call. I don’t imagine having to create documentation to explain every developer this intricate behavior when it should work just flagging the layout. Personally, it took me a lot of time realizing why the components were executing a lot of times unexpectedly. And there are more things that are affecting the rendering of our components, like event-callbacks. I’m looking to get rid of them because they also trigger the rendering unexpectedly. For example listening to a click performs a rendering.

Having a StateHasChangedAsync built-in in the platform is a must, also having a method to perform the render synchronously is a must. I understand your point about back-compatibility, but the current StateHasChanged should be discouraged to be used, an Obsolete attribute would explain developers what to use instead, and the documentation should say not to use it because of all the problems it brings.

@arivoir Your C# only case makes perfect sense. You wouldn’t want to block the rendering for 10 seconds just because multiple long-running tasks haven’t completed. We’d want to see the UI update incrementally in that case.

Nobody said the rendering would be blocked for 10 seconds, that would be a terrible bug. The point is the second call should just perform 1 render.

I agree. I had thought of a timer as another case where a render is not the result of an event dispatch, but async is a nice find as well. This deserves a closer look quickly.

BTW, this has nothing to do with the fact that it’s async. Everything is executed on the current synchronizationcontext, there is no dispatching. You can even rewrite the UpdateComponent method as:

 public void UpdateTheComponent()
    {
        InvokeAsync(() =>
        {
            for (int i = 0; i < 100; i++)
            {
                StateHasChanged();
            }
        }
    }

And it will still happen, even though we are sure now that StateHasChanged happens within the render context.

It’s a proof of principle.

In order to save processing time When calling StateHasChanged many times Then the component should only be marked as needing to be rerendered And should render only once when the next render process occurrs

Here is a more simple demonstration of the problem

In _Host.cshtml <head> add

  <script>
    window.callBackBlazor = function (blazorComponent) {
      blazorComponent.invokeMethodAsync("ExecuteStateHasChangedInALoop");
		}
  </script>

Then click the button in the following component

@page "/"
@inject IJSRuntime JSRuntime

@{ 
	System.Diagnostics.Debug.WriteLine(DateTime.UtcNow + " Rendering");
}
<button @onclick=CallJs>Call JS</button>

@code
{
	DotNetObjectReference<Index> ObjRef;

	protected override void OnAfterRender(bool firstRender)
	{
		ObjRef = DotNetObjectReference.Create(this);
	}

	private async Task CallJs()
	{
		await JSRuntime.InvokeVoidAsync("callBackBlazor", ObjRef).ConfigureAwait(false);
	}

	[JSInvokable]
	public async Task ExecuteStateHasChangedInALoop()
	{
		for (int i = 0; i < 3; i++)
			StateHasChanged();
	}
}

It seems that neither void or async/await or await Task.Delay(1) make any difference.

@SQL-MisterMagoo StateHasChanged is called directly from a C# loop