Partially fix #819

[tvardero]

@jbogard, I have added two unit tests that cover the situation

[tvardero]

Sometimes I think that the issue should not be resolved on Mediator end, as some other containers (not MS.Ext.DI, but for example Lamar) does support contravariance and it was working before the proposed patch.

This patch should only be applied to MS.Ext.DI and not all other containers, @asimmon could you suggest me how to do such? I don't want to add another layer of abstraction for service resolving, maybe there is another way ...

cc @jbogard

[jbogard]

Yeah I'm more inclined to let 3rd-party containers take care of this. They'll do the job much better than I can, and I've already taken on a bit much by adding the scanning for handlers.

[tvardero]

For MS.Ext.DI I have already (re-)opened the issue: https://github.com/dotnet/runtime/issues/82372 They already said "wontfix" back in 2016, but I have hope for such change today.

[asimmon]

@tvardero Before 12.x, MediatR was container-agnostic-ish. If you look at v11.1.0/src/MediatR/Mediator.cs, there was a ServiceFactory acting as a DI abstraction layer.

In 12.x, IServiceProvider from Ms.Ext.DI is the built-in way of resolving dependencies. I personally think this a valid approach as ASP.NET Core, hosting extensions and many Microsoft and third-party libraries rely on this by default. It's part of the .NET ecosystem. Too ensure projects longevity and smooth migrations to more recent .NET versions in the next 5 years at least, developers should rely on Ms.Ext.DI.

[asimmon]

Also @tvardero I looked deeper at your original issue #819 and don't think there's anything to fix here.

Disclaimer: I also use MediatR to implement the CQRS pattern.

You're implementing the command pattern using MediatR's INotification and INotificationHandler. I may be wrong but I don't think that's the intended way of implementing commands. I would expect developers to use IRequest and IRequestHandler instead.

Your original problem seems like a pattern-related problem and there are ways to support your use case by using MediatR's built-in API: IRequest, IRequestHandler, behaviors, validators, etc. I can think of at least two ways to implement what you're trying to do with these APIs.

[tvardero]

I would expect developers to use IRequest and IRequestHandler instead.

Isn't behavior of the IRequest (or we could say IRequest<void> / IRequest<Unit>) and it's IRequestHandler will be the same as with INotification and it's handler? Except for exceptions handling probably, INotification should work as fire-and-forget. Anyway, the #819 was about resolving a handler. I haven't done any changes to IRequestHandler<in TCommand> / IRequestHandler<in TCommand, out TResult> resolution with MS.Ext.DI, but I assume there will be the same issue with contravariance.

[jbogard]

Isn't behavior of the IRequest (or we could say IRequest<void> / IRequest<Unit>) and it's IRequestHandler will be the same as with INotification and it's handler? Except for exceptions handling probably, INotification should work as fire-and-forget.

No, it's not. There is an expectation of a single handler for a single request, and 0-n handlers for a single notification. This is enforced by how we use the container, it will throw an exception without a request handler, and do nothing without a notification handler.

Generic variance makes sense in the context of notification handlers and behaviors. NOT in request handlers. Those should be closed generics, never open.

[asimmon]

So, I did the benchmarks by isolating the path that resolve the INotificationHandler<T> (so just before we create the wrappers) to measure the impact of your changes.

TL;DR: it is 10x slower and consumes also 10x more memory.

|      Method |       Mean |    Error |   StdDev | Ratio | RatioSD |   Gen0 | Allocated | Alloc Ratio |
|------------ |-----------:|---------:|---------:|------:|--------:|-------:|----------:|------------:|
| BeforePR855 |   156.5 ns |  1.79 ns |  1.68 ns |  1.00 |    0.00 | 0.0162 |     136 B |        1.00 |
|  AfterPR855 | 1,575.5 ns | 14.58 ns | 13.64 ns | 10.07 |    0.13 | 0.1659 |    1392 B |       10.24 | <--- This PR

The benchmark code can be found here.

While I completely agree that you probably don't need to worry about MediatR's performance, in my opinion this PR is a big no no. I'd like your opinion about this @jbogard if you're keen.

If I had to summarize:

• 10x slower and 10x more allocations due to reflection
• Usage of SelectMany() on IServiceProvider.GetServices which frankly seems odd
• Handler order disrupted. Usually IServiceProvider.GetServices takes care of returning services in their registration order, but here we interfere with that.

[github-actions[bot]]

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

[github-actions[bot]]

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