EventCallback.Equals is not giving correct results

[JelleHissink]

Is there an existing issue for this?

• [X] I have searched the existing issues

Describe the bug

We noticed after upgrading our blazor application to .net 8.0 that some EventCallback keep being modified. Eventhough the value was the same, the same target component, the same method. The generated code that renders the component seems to fabricate a new MulticastDelegate, having the same Type, Target and Method. The implementation of EventCallback uses Object.ReferenceEquals, so it concludes it is different every render.

Expected Behavior

The component should see EventCallbacks as equal when the delegate and Target are the same. MulticastDelegate has the Equals method overridden, so use that instead of ReferenceEquals.

Steps To Reproduce

I will make a PR with a fix

Exceptions (if any)

No response

.NET Version

8.0.101

Anything else?

No response

[SteveSandersonMS]

We noticed after upgrading our blazor application to .net 8.0

Are you saying the behavior changed in .NET 8 compared with some earlier framework? If so, can you show some sample code that works differently in (say) .NET 7 compared with .NET 8?

The component should see EventCallbacks as different when the value is the same.

Is that a typo? Do you mean should not instead of should?

[JelleHissink]

@SteveSandersonMS good catch, that was a typo. I fixed the equals.

The previous EventCallback.Equals fell back to the default Equals implementation. It is now implemented in .Net 8. So up until .net 7 it used the MemberwiseEquals because it is a struct.

[SteveSandersonMS]

Can you give an example of code where you would be using this? Normally we only expect EventCallback instances to be compared internally by Blazor's renderer, which is what the new implementation is optimized around.

If we change it to use .Equals then it will only make a different to the renderer if the delegate is uncapturing. AFAIK for capturing delegates the Target will always be different on successive renders. It's possible that delegates could be noncapturing often enough to make this worthwhile but it does require some consideration.

[JelleHissink]

A loading wrapper that does some error handling and progress that has an OnLoad and then a Render. It goes something like this: ( I omitted a lot of code for error handling, retry in case of failure etc)

@if (_loading) {
    <DisplayLoadingIndicator />
} else {
    @ChildContent
}
@code {
    [Parameter, EditorRequired] public required EventCallback OnLoad { get; set; }
    [Parameter, EditorRequired] public required RenderFragment ChildContent { get; set; }

    private bool _loaded = false;
    private  EventCallback _previousOnLoad;

    protected override async Task OnParametersSetAsync()
    {
        await base.OnParametersSetAsync();
        if (!_previousOnLoad.Equals(OnLoad))
        {
            _previousOnLoad = OnLoad;
            await Refresh();
        }
    }

    public async Task Refresh()
    {
        await OnLoad.InvokeAsync();
        StateHasChanged();
        _loaded = true;
    }
}

When a component uses this, OnLoad is always different by the razor generator even though the delegate.Equals() would return true. We see this component being rendered over and over. If the parent was rerendered, OnParametersSetAsync() was called every time, and thus we do something. But in effect the parameter was equal, Receiver, Delegate.Target, Delegate.Method are all equal. For us there is no way to determine if the callback has changed (eg. if the parent component is changing).

[JelleHissink]

Hmm... I see your point in uncaptered events, we only directly point to a method, so for us the Target of the delegate would be equal. We are not doing Onload="@(() => LoadData())" but OnLoad=LoadData.

[ghost]

Thanks for contacting us.

We're moving this issue to the .NET 9 Planning milestone for future evaluation / consideration. We would like to keep this around to collect more feedback, which can help us with prioritizing this work. We will re-evaluate this issue, during our next planning meeting(s). If we later determine, that the issue has no community involvement, or it's very rare and low-impact issue, we will close it - so that the team can focus on more important and high impact issues. To learn more about what to expect next and how this issue will be handled you can read more about our triage process here.

[javiercn]

Discussed this with @SteveSandersonMS and we are planning to make a change in 9.0 for this to use Equals instead of ReferenceEquals

[SteveSandersonMS]

The PR at https://github.com/dotnet/aspnetcore/pull/53362 looks good for that

[JelleHissink]

Thanks, I'll make a story for us to implement this again when .net 9 comes along ;-)

[JelleHissink]

I just managed to mess the PR up, I have created a new PR to main #53395