Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
neozhu opened this issue Mar 21, 2023 · 17 comments
Closed

Comments

@neozhu
Copy link

neozhu commented Mar 21, 2023

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
Copy link

billrob commented Apr 5, 2023

Are you able to attach a sln showing the behavior?

@MichaelHochriegl
Copy link

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
Copy link

mrpresidentjk commented May 16, 2023

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:

  • public class HandlerOne: INotificationHandler<T> where T : INotificationChangeIntegrationEvent
  • public class HandlerTwo : INotificationHandler<NotificationCreatedIntegrationEvent>

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
Copy link
Author

neozhu commented May 16, 2023

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

@mesakomarevich
Copy link

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
Copy link
Owner

jbogard commented Jun 1, 2023

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

@mesakomarevich
Copy link

@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
Copy link

jscarle commented Jul 13, 2023

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
Copy link

Here is the issue on dotnet side: dotnet/runtime#87017

In this issue there are two MRs that fix that problem:
dotnet/runtime#80410
dotnet/runtime#52035

@jscarle
Copy link

jscarle commented Jul 13, 2023

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

@Mayron
Copy link

Mayron commented Aug 19, 2023

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
Copy link
Owner

jbogard commented Aug 19, 2023

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

@Mayron
Copy link

Mayron commented Aug 21, 2023

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
Copy link

@Mayron The bug is in .Net itself, you can see the issue and associated pull requests in my post here #884 (comment)

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
Copy link

Mayron commented Aug 21, 2023

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
Copy link

Sebazzz commented Sep 1, 2023

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
Copy link
Owner

jbogard commented Sep 1, 2023

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.

@jbogard jbogard closed this as completed Sep 1, 2023
@jbogard jbogard closed this as not planned Won't fix, can't repro, duplicate, stale Sep 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants