mikoskinen / Blazor.EventAggregator

Lightweight Event Aggregator for Blazor (Razor Components).
MIT License
120 stars 23 forks source link

Not working as expected, cause too much render on the component which suscribe #4

Open julienGrd opened 5 years ago

julienGrd commented 5 years ago

Hi @mikoskinen and thanks for your work !

However i see a huge problem in your library. Indeed i faced a performance issue with my app, and i see some of my components are definitively render too much (https://github.com/aspnet/AspNetCore/issues/14684) i debug this and i see each time one of the event aggregator (whatever the parameters) publish, all of the components wich suscribe (whatever the parameters) are re-loaded (i see them with put some trace on the ShouldRender when the value is true, when i traced back the call stach i arrive on the publish of components).

I precise i see this behavior is client-side (web assembly)

so if you have a component C1 wich publish the a message of type T1 and and component C2 which suscribe of message of type T2, C2 is reloaded when C1 publish, which make no sense, normally the type is used to isolate the different case.

In my case, the components was reloaded 8 time instead of 1. I don't take time to debug your code so i don't know what is causing this issue. However, i see you try to called automatically the StateAsChanged in your code, i think it's a bad idea, In my case sometime i don't want to relad my components (some are very huge) and can be a performance problem event without this bug. at least provide an option to disable it (or better, as a parameter of the publish).

Maybe i can make some PR if you need help.

Good luck

Julien

mikoskinen commented 5 years ago

Thanks Julien for the report. I wonder, are you able to provide a simple repro for the issue?

As you mentioned, the library shouldn't be calling StateHasChanged automatically anymore. It's a leftover from the early days of Blazor to help get around around some early bits.

julienGrd commented 5 years ago

Thanks for your reactivity !

i push a simple repro here : https://github.com/julienGrd/BlazorWebAssemblyBugEventAggregator

Hope it help, good luck !

rborosak commented 5 years ago

@mikoskinen The problem is in PublishAsync method of EventAggregator.. First you get all handlers

handlersToNotify = _handlers.ToArray();

Then you get and execute all the tasks from handlers that can handle published message

var tasks = handlersToNotify.Select(h => h.Handle(messageType, message));
await Task.WhenAll(tasks);

And the problem is in the next line.

foreach (var handler in handlersToNotify.Where(x => !x.IsDead))

In this foreach loop you are invoking StateHasChanged for every live handler so every component gets rerendered.

I think you should filter out the handler at the begining handlersToNotify = _handlers.Where(x => x.Handles(messageType)).ToArray();

and then change from var deadHandlers = handlersToNotify.Where(h => h.IsDead).ToList(); to var deadHandlers = _handlers.Where(h => h.IsDead).ToList(); so you check if there are any dead handler no matter of message type