jbogard / MediatR

Simple, unambitious mediator implementation in .NET
Apache License 2.0
11.1k stars 1.18k forks source link

Handle method are executed twice in INotificationHandler in a specific scenario. #884

Closed neozhu closed 1 year ago

neozhu commented 1 year ago

I have a question regarding the behavior of the Handle method in INotificationHandler in a specific scenario.

I have an ApprovedEventHandler.cs class which is a NotificationHandler that sends email notifications and uses a generic type for reusability. Here is the code snippet:

public abstract class DomainEvent: INotification
{
    protected DomainEvent()
    {
        DateOccurred = DateTimeOffset.UtcNow;
    }
    public bool IsPublished { get; set; }
    public DateTimeOffset DateOccurred { get; protected set; }
}
public  class ApprovedEvent<T>: DomainEvent
{
    public ApprovedEvent(T entity)
    {
        Entity = entity;
    }

    public T Entity { get; }
}
public class DomainEventNotification<TDomainEvent> : INotification where TDomainEvent : DomainEvent
{
    public DomainEventNotification(TDomainEvent domainEvent)
    {
        DomainEvent = domainEvent;
    }

    public TDomainEvent DomainEvent { get; }
}

public class ApprovedEventHandler<T> : INotificationHandler<DomainEventNotification<ApprovedEvent<T>>> where T : AuditableEntity
{

    public ApprovedEventHandler()
    {
    }
    public async Task Handle(DomainEventNotification<ApprovedEvent<T>> notification, CancellationToken cancellationToken)
    {
        var domainEvent = notification.DomainEvent;

    }
}

The issue I am encountering is that when I exclude this ApprovedEventHandler.cs class from the project, all other NotificationHandlers are executed twice (subscribed twice) after compiling and adding the class back in.

I would appreciate it if someone could explain to me why this is happening. Although I have found a workaround for this problem, I am still curious to understand the underlying principle. Thank you.

public class AccessRequestCreatedEvent: DomainEvent
{
    public AccessRequestCreatedEvent(AccessRequest item)
    {
        Item = item;

    }

    public AccessRequest Item { get; }
}
public class AccessRequestCreatedEventHandler: INotificationHandler<AccessRequestCreatedEvent>
{

    public AccessRequestCreatedEventHandler(

            )
    {

    }
    public async Task Handle(AccessRequestCreatedEventnotification, CancellationToken cancellationToken)
    {
        var request = notification.Item;

    }
}
billrob commented 1 year ago

Are you able to attach a sln showing the behavior?

MichaelHochriegl commented 1 year ago

I ran into the same problem today and created a quick reproduction repository that you can find here.

It's a simple minimal API that has swagger build in. There is a generic handler for the DummyEvent, so if you hit the /createDummy endpoint for example you can see in the logs that the handler DummyCreatedEventHandler get's executed twice, the generic handler DummyEventHandler doesn't get executed at all.

mrpresidentjk commented 1 year ago

I have experienced the same issue as the above.

I was handling an event called NotificationCreatedIntegrationEvent which implemented an interface called INotificationChangeIntegrationEvent with the following handlers:

The generic handler was causing the other handler to be called twice.

My use case was very similar to the OP's where I wanted one handler to handle create, delete and update events in order to notify the client of changes via a websocket but only send emails on create (HandlerTwo).

I would be happy to have a look at this issue but I may not necessarily have the time. @neozhu I'd be keen to hear how you got around the issue.

I can pretty easily implement handlers for each event but this is a lot of classes.

Cheers!

neozhu commented 1 year ago

You're right, I've decided to give up using generics INotificationHandler for now.

mesakomarevich commented 1 year ago

Hey, all, this is not an issue with MediatR, but a bug in the Microsoft DI functionality for resolving an IEnumerable<T> of services. I hope to have a PR in to fix this soon, when this issue is merged, we will have to discuss updating the package reference in MediatR to resolve this issue.

jbogard commented 1 year ago

My last PR for Microsoft DI was open for 5 years (literally) before merging so...good luck lol.

mesakomarevich commented 1 year ago

@jbogard well, if I have to get to the annoying level of persistence with this bug, I will. That being said, you may want to update the container feature support page to indicate that Microsoft DI doesn't always play nice with notification handlers.

jscarle commented 1 year ago

Hey, all, this is not an issue with MediatR, but a bug in the Microsoft DI functionality for resolving an IEnumerable<T> of services. I hope to have a PR in to fix this soon, when this issue is merged, we will have to discuss updating the package reference in MediatR to resolve this issue.

You should add a link to your PR in this thread.

MichaelHochriegl commented 1 year ago

Here is the issue on dotnet side: https://github.com/dotnet/runtime/issues/87017

In this issue there are two MRs that fix that problem: https://github.com/dotnet/runtime/pull/80410 https://github.com/dotnet/runtime/pull/52035

jscarle commented 1 year ago

I'm so glad that Microsoft open sourced dotnet. This never would have been adressed, let alone fixed, in the past.

Mayron commented 1 year ago

Hey, all, this is not an issue with MediatR, but a bug in the Microsoft DI functionality for resolving an IEnumerable<T> of services. I hope to have a PR in to fix this soon, when this issue is merged, we will have to discuss updating the package reference in MediatR to resolve this issue.

I've linked some sample code to recreate my issue that sounds like the same bug: https://github.com/Mayron/Demo-NotificationService

I mentioned that even if it's a Microsoft issue, hopefully, MediatR can be updated to provide a workaround to make notification handlers work as expected for older .NET versions containing this bug and by the sounds of your reply it seems likely 😁

jbogard commented 1 year ago

I wasn't planning on updating anything in MediatR if that's what you meant.

Mayron commented 1 year ago

I wasn't planning on updating anything in MediatR if that's what you meant.

Ah, my mistake! I misread someone else's comment and got confused 🤦.

I updated my repository to .NET 8 preview and it has fixed the issue.

Is updating to .NET 8 the only way to fix this? My concern is that the actual bug is in a very large codebase for an enterprise application. Usually updating .Net versions requires a lot of testing and risk management.

Just want to know my options :)

MichaelHochriegl commented 1 year ago

@Mayron The bug is in .Net itself, you can see the issue and associated pull requests in my post here https://github.com/jbogard/MediatR/issues/884#issuecomment-1633736060

And as the fix is in .Net8 that's why you don't see the issue after using .Net8. And yes, updating to .Net8 is for now the only way to fix this. David Fowler stated in the linked issue above that MS will consider a backport of this to .Net6/7 once the change is battletested with .Net8.

Mayron commented 1 year ago

David Fowler stated in the linked issue above that MS will consider a backport of this to .Net6/7 once the change is battletested with .Net8.

@mesakomarevich This is what I was hoping for, thank you! I'm happy to wait because to temporarily solve the issue I set the service provider's ValidateOnBuild option to false explicitly and that has fixed the issue; the notification handler only fires once.

It's not great because that validation is very useful, but it's the best temporary solution I can think of so hopefully this works for others too.

Sebazzz commented 1 year ago

I mentioned that even if it's a Microsoft issue, hopefully, MediatR can be updated to provide a workaround to make notification handlers work as expected for older .NET versions containing this bug and by the sounds of your reply it seems likely 😁

There isn't really workaround if you want to use generic services, so I don't think that changing anything in MediatR would help.

jbogard commented 1 year ago

Yes, and I don't plan on trying to work around limitations or bugs in the stock MS container. The real fix is to swap containers to something that doesn't have this bug, like Lamar, Autofac etc.