jbogard / MediatR

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

PipelineBehavior registration not working properly #944

Closed makercob closed 4 months ago

makercob commented 11 months ago

Given the request, the handler and the behaviors:

public sealed record FooRequest : IRequest;

public sealed class FooRequestHandler : IRequestHandler<FooRequest> {
    public FooRequestHandler(ILogger<FooRequestHandler> logger) {
        this.logger = logger;
    }

    readonly ILogger logger;

    public Task Handle(FooRequest request, CancellationToken cancellationToken) {
        logger.LogInformation("Invoked");
        return Task.CompletedTask;
    }
}

sealed class ClosedBehavior : IPipelineBehavior<FooRequest, Unit> {
    public ClosedBehavior(ILogger<ClosedBehavior> logger) {
        this.logger = logger;
    }

    readonly ILogger logger;

    public Task<Unit> Handle(FooRequest request, RequestHandlerDelegate<Unit> next, CancellationToken cancellationToken) {
        logger.LogInformation("Invoked");
        return next();
    }
}

sealed class OpenBehavior<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse>
    where TRequest : notnull {
    public OpenBehavior(ILogger<OpenBehavior<TRequest, TResponse>> logger) {
        this.logger = logger;
    }

    readonly ILogger logger;

    public Task<TResponse> Handle(TRequest request, RequestHandlerDelegate<TResponse> next, CancellationToken cancellationToken) {
        logger.LogInformation("Invoked");
        return next();
    }
}

The closed behavior is invoked twice while the open behavior is ignored, if the closed behavior is registered before the open behavior, using the following code:

builder.Services.AddMediatR(
    config => {
        config.RegisterServicesFromAssembly(
            Assembly.GetExecutingAssembly()
        );

        config.AddBehavior<ClosedBehavior>();
        config.AddOpenBehavior(typeof(OpenBehavior<,>));
    });

Program outputs:

info: ServerBlazorApp.Requests.ClosedBehavior[0]
      Invoked
info: ServerBlazorApp.Requests.ClosedBehavior[0]
      Invoked
info: ServerBlazorApp.Requests.FooRequestHandler[0]
      Invoked

All is working as expected, if the closed behavior is registered after the open behavior, using the following code:

builder.Services.AddMediatR(
    config => {
        config.RegisterServicesFromAssembly(
            Assembly.GetExecutingAssembly()
        );

        config.AddOpenBehavior(typeof(OpenBehavior<,>));
        config.AddBehavior<ClosedBehavior>();
    });

Program outputs:

info: ServerBlazorApp.Requests.OpenBehavior[0]
      Invoked
info: ServerBlazorApp.Requests.ClosedBehavior[0]
      Invoked
info: ServerBlazorApp.Requests.FooRequestHandler[0]
      Invoked
makercob commented 11 months ago

Mediate 12.1.1 is used.

attiqeurrehman commented 11 months ago

+1 same issue with me

lukasz-krzykowski commented 11 months ago

I have the same problem. At first I've suspected it was bug in the MediatR 10 that I've used, so I migrated to latest (12.1.1) but issue still occurs, exactly as described by @makercob

lukasz-krzykowski commented 11 months ago

I was able to get workaround that problem with explicitly registering the OpenBehavior for the particular command like so:

                    cfg.AddBehavior<ClosedBehavior>();
                    cfg.AddOpenBehavior(typeof(OpenBehavior<,>));
                    cfg.AddBehavior<OpenBehavior<FooRequest, Unit>>();

It renders some quirks, that I'm able to live with for now like:

jbogard commented 11 months ago

I can't reproduce this. Can you create a small complete sample that shows this behavior? This is what I've tried:

[Fact]                                                           
public async Task Should_register_correctly()                    
{                                                                
    var services = new ServiceCollection();                      
    services.AddMediatR(cfg =>                                   
    {                                                            
        cfg.RegisterServicesFromAssemblyContaining<FooRequest>();
        cfg.AddBehavior<ClosedBehavior>();                       
        cfg.AddOpenBehavior(typeof(Open2Behavior<,>));           
    });                                                          
    var logger = new Logger();                                   
    services.AddSingleton(logger);                               
    services.AddTransient(typeof(IBlogger<>), typeof(Blogger<>));
    var provider = services.BuildServiceProvider(true);          

    var mediator = provider.GetRequiredService<IMediator>();     
    var request = new FooRequest();                              
    await mediator.Send(request);                                

    logger.Messages.ShouldBe(new []                              
    {                                                            
        "Invoked Closed Behavior",                               
        "Invoked Open Behavior",                                 
        "Invoked Handler",                                       
    });                                                          
}

public interface IBlogger<T>                          
{                                                     
    IList<string> Messages { get; }                   
}                                                     

public class Blogger<T> : IBlogger<T>                 
{                                                     
    private readonly Logger _logger;                  

    public Blogger(Logger logger)                     
    {                                                 
        _logger = logger;                             
    }                                                 

    public IList<string> Messages => _logger.Messages;
}                                                     
makercob commented 11 months ago

@jbogard Cannot reproduce the issue in UnitTest either. Here is the minimal API test code that can reproduce the issue.

using MediatR;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddMediatR(
    cfg => {
        cfg.RegisterServicesFromAssemblyContaining<FooRequest>();
        cfg.AddBehavior<ClosedBehavior>();
        cfg.AddOpenBehavior(typeof(OpenBehavior<,>));
    }
);

var app = builder.Build();

app.MapGet(
    "/",
    async (IMediator mediator) => {
        await mediator.Send(new FooRequest());
        return "OK";
    });

app.Run();

class FooRequest : IRequest {
    class Handler : IRequestHandler<FooRequest> {
        public Handler(ILogger<Handler> logger) {
            this.logger = logger;
        }

        readonly ILogger<Handler> logger;

        public Task Handle(FooRequest request, CancellationToken cancellationToken) {
            logger.LogInformation("FooRequestHandler");
            return Task.FromResult(0);
        }
    }
}

class ClosedBehavior : IPipelineBehavior<FooRequest, Unit> {
    public ClosedBehavior(ILogger<ClosedBehavior> logger) {
        this.logger = logger;
    }

    readonly ILogger<ClosedBehavior> logger;

    public async Task<Unit> Handle(
        FooRequest request,
        RequestHandlerDelegate<Unit> next,
        CancellationToken cancellationToken) {
        logger.LogInformation("ClosedBehavior");
        return await next().ConfigureAwait(false);
    }
}

class OpenBehavior<TRequest, TResponse> : IPipelineBehavior<TRequest, TResponse>
    where TRequest : notnull {
    public OpenBehavior(ILogger<OpenBehavior<TRequest, TResponse>> logger) {
        this.logger = logger;
    }

    readonly ILogger<OpenBehavior<TRequest, TResponse>> logger;

    public async Task<TResponse> Handle(
        TRequest request,
        RequestHandlerDelegate<TResponse> next,
        CancellationToken cancellationToken) {
        logger.LogInformation("OpenBehavior");
        return await next().ConfigureAwait(false);
    }
}

Invoking the API produces outputs as follow:

info: ClosedBehavior[0]
      ClosedBehavior
info: ClosedBehavior[0]
      ClosedBehavior
info: FooRequest.Handler[0]
      FooRequestHandler
jbogard commented 11 months ago

I was able to reproduce this. It's a bug in the MS DI container:

    [Fact]
    public async Task Should_register_correctly()
    {
        var services = new ServiceCollection();
        services.AddMediatR(cfg =>
        {
            cfg.RegisterServicesFromAssemblyContaining<FooRequest>();
            cfg.AddBehavior<ClosedBehavior>();
            cfg.AddOpenBehavior(typeof(Open2Behavior<,>));
        });
        var logger = new Logger();
        services.AddSingleton(logger);
        services.AddSingleton(new MediatR.Tests.PipelineTests.Logger());
        services.AddSingleton(new MediatR.Tests.StreamPipelineTests.Logger());
        services.AddSingleton(new MediatR.Tests.SendTests.Dependency());
        services.AddSingleton<System.IO.TextWriter>(new System.IO.StringWriter());
        services.AddTransient(typeof(IBlogger<>), typeof(Blogger<>));
        var provider = services.BuildServiceProvider(new ServiceProviderOptions
        {
            ValidateOnBuild = true // This is true in Development mode, false otherwise
        });
        //var provider = services.BuildServiceProvider(true);

        var mediator = provider.GetRequiredService<IMediator>();
        var request = new FooRequest();
        await mediator.Send(request);

        logger.Messages.ShouldBe(new []
        {
            "Invoked Closed Behavior",
            "Invoked Open Behavior",
            "Invoked Handler",
        });
    }

But it's our old friend https://github.com/dotnet/runtime/issues/65145 here. It's a duplicate of https://github.com/jbogard/MediatR.Extensions.Microsoft.DependencyInjection/issues/118 but I'll keep this one open as I've archived that other repo.

jbogard commented 11 months ago

The next version of MediatR will target .NET 8-focused libraries, and this issue should be fixed then. I have a unit test that I'll keep that will verify it.

jbogard commented 11 months ago

Confirmed that the test passes in the latest pre-release (8.0.0-preview.7.23375.6)

github-actions[bot] commented 9 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

lukasz-krzykowski commented 9 months ago

I think we should not close it before it's merged - since we already have working solution

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

lukasz-krzykowski commented 6 months ago

Since dotnet 8 is already released - can we merge this fix @jbogard ?

github-actions[bot] commented 4 months ago

This issue is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 14 days.

github-actions[bot] commented 4 months ago

This issue was closed because it has been stalled for 14 days with no activity.