jbogard / MediatR

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

Reduced IMediator interface #116

Closed jbogard closed 7 years ago

jbogard commented 7 years ago
public interface IMediator
{
    Task<TResponse> SendAsync<TResponse>(IRequest<TResponse> request, CancellationToken cancellationToken = default(CancellationToken));

    Task SendAsync(IRequest request, CancellationToken cancellationToken = default(CancellationToken));

    Task PublishAsync(INotification notification, CancellationToken cancellationToken = default(CancellationToken));
}

I'd keep all the existing handler interfaces, and inside the Mediator class decide which one is the right one to instantiate through cascading checks and then a cache to the right request.

remcoros commented 7 years ago

This means you'll leave the sync-over-async implementation to the consumers of IMediator?

jbogard commented 7 years ago

@remcoros I'll leave all the handlers alone. You can have IRequestHandler, IAsyncRequestHandler etc.

jbogard commented 7 years ago

ping @khellang

remcoros commented 7 years ago

@jbogard I mean, the consumer of IMediatr can be non-async.

So with this new interface, we'll have to wrap the SendAsync calls in a synchronous api when calling from non-async code.

Either:

jbogard commented 7 years ago

@remcoros yes. Supporting both models is annoying to say the least, and since calling frameworks are generally async-friendly (MVC back many versions, WPF w/ any modern MVVM frameworks, Xamarin too, messaging frameworks of NSB and MassTransit), it's likely users calling code can be easily async.

But I've only confirmed this with our apps that use MediatR, where are you calling into it?

remcoros commented 7 years ago

I use mediatr from windows services (topshelf). Also, asp.net mvc 5 child actions do not (and will not) support async.

In those cases I either wrap async api's using Task.Run or Wait() and be super careful of deadlocks.

I like it clean and simple too, just confirming you thought this trough. If a true sync api is possible I would support it though. I personally don't mind the three extra methods.

Either way is fine, just something to keep in mind when migrating from v2.

jbogard commented 7 years ago

The problem is with the addition of pipeline support, I'd have to support 3 pipelines (sync/async/async w/cancel), and have to resort to the same things you saw above. With so many things supporting async these days, it's easier to just allow that model throughout, just like NSB did with 6.0. It's async-only too.

abatishchev commented 7 years ago

Recently I discovered that Entity Framework 6 works terribly (basically doesn't) with async transactions, so either one or another. Spent a week but ended up with Task.Run().Wait().

khellang commented 7 years ago

Having "async with cancel" is just wierd. That's just async, IMO :wink: Also, MediatR 2.x will still exist with sync-support :smile:

khellang commented 7 years ago

Also, if you're dropping the sync overloads, I think we should drop the sync handler interfaces and just have sync abstract base classes instead. That way it'll be easier to register and resolve handlers and you're using the same interface all over.

abatishchev commented 7 years ago

I'd drop CancellationToken parameter too, since it's not automatic (as should be) so what's the point? I believe 95% don't use it at all, 4% use just because there's an overload but don't actually cancel it, so it's only for 1% left. Of course I made up all these numbers.

And I support to drop all sync-related infrastructure including interfaces. Again, what's the point to keep if IMediator won't support them directly anymore?

khellang commented 7 years ago

I always and only use CancellationToken overloads. Without them, I won't use MediatR. You just get the token from MVC and pass it down to the API, then you get cancellation for free when requests are aborted. The MS API recommendations is to make them optional anyway, so it doesn't really matter. In your handler, it's up to you if you want to use it or not.

khellang commented 7 years ago

Oh, and @abatishchev... As I just mentioned; it is automatic :smile:

jbogard commented 7 years ago

The base classes are problematic, we already frequently use base handler classes for common behavior and this would prevent us from doing so. The concrete classes have the interface implementation.

khellang commented 7 years ago

You can compose behavior :wink:

abatishchev commented 7 years ago

@khellang token is cancelled automatically by the caller, yes. Task is not cancelled by the framework automatically. Instead library has to perform cancellation manually and implement it properly. Doesn't it?

khellang commented 7 years ago

The token is signalled by the framework, yes. Automatically. When the user agent aborts the request. I'm not sure what you're talking about :confused:

abatishchev commented 7 years ago

The framework doesn't cancel a task, library has to do it. And people tend to implement it differently, sometimes incorrectly. So I don't use it. But this is unrelated discussion.

jbogard commented 7 years ago

A separate question would be around registration, a marker interface was proposed there but I don't think it really helps that much.

khellang commented 7 years ago

The framework doesn't cancel a task, library has to do it.

Yes it does. Here's the exact line where it signals the token. Now, can you, once again, stop talking about stuff you have no clue about? I'm tired of the "no, it doesn't" "yes, it does" "no, it doesn't" "yes, it absolutely does" thing... This isn't the first time you've been trolling this issue tracker.

jbogard commented 7 years ago

See #117 for a working example

abatishchev commented 7 years ago

you have no clue about

Who's saying? You? OMFG. I LOL"D.

If cancellation is requested before the task begins execution, the task does not execute. Instead it is set to the Canceled state and throws a TaskCanceledException exception.

https://msdn.microsoft.com/en-us/library/hh160373(v=vs.110).aspx http://stackoverflow.com/questions/4783865/how-do-i-abort-cancel-tpl-tasks

RTFM or GTFO.

You're smart and experienced person but your manners are terrible.

jbogard commented 7 years ago

Well it's merged now anyway.