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

Added option to enable PublishAsync to invoke handlers sequentially #101

Closed
wants to merge 2 commits into from

Conversation

@samueldjack
Copy link

@samueldjack samueldjack commented Sep 1, 2016

This helps to address #98 by adding a configuration option to enable invoking async notification handlers sequentially rather than in parallel.

{
var notificationHandlers = GetNotificationHandlers(notification)
var tasks = notificationHandlers
.Select(handler => handler.Handle(notification, cancellationToken))
.ToArray();

This comment has been minimized.

@abatishchev

abatishchev Sep 1, 2016

Please drop ToArray() (here and elsewhere). This is not needed and actually can lead to a wrong behavior. Send the IEnumerable directly to Task.WhenAll() and it will do the needful to run them in parallel properly.

This comment has been minimized.

@khellang

khellang Sep 1, 2016
Contributor

This is not needed and actually can lead to a wrong behavior.

This is false. The source clearly shows that both WhenAll(IEnumerable<Task>) and WhenAll(Task[]) ends up in the same InternalWhenAll method after checking for null tasks.

Send the IEnumerable directly to Task.WhenAll() and it will do the needful to run them in parallel properly.

As mentioned above, it'll do the exact same thing as doing ToArray() before passing it in. It doesn't matter.

This comment has been minimized.

@abatishchev

abatishchev Sep 1, 2016

False or true, anyway let's be more friendly, right? I'm basing my statement on my experience that calling ToArray() kick-offs these tasks before they're reaching Task.WhenAll() what doesn't help much during debugging. Thanks for pointing out to the source code. But still I'd pass IEnumerable directly and let TPL do its work.

This comment has been minimized.

@khellang

khellang Sep 1, 2016
Contributor

False or true, anyway let's be more friendly, right?

I'm sorry. It wasn't my intention to be unfriendly 😕 I just wanted to point out that what you said was incorrect and based on false assumptions. @samueldjack did nothing wrong by calling ToArray before passing the tasks to WhenAll, or simply leaving the code as-is. The sooner the tasks are "kicked off", the better, IMO 😄

I'm basing my statement on my experience that calling ToArray() kick-offs these tasks before they're reaching Task.WhenAll() what doesn't help much during debugging.

Yes, the enumerable is enumerated when calling ´ToArray´, but does it matter if that happens before the call to WhenAll? It's only a matter of time before ToArray is called anyway... If anything, you could argue that passing an array is marginally faster than calling the enumerable overload, because of fewer checks. I'm not sure how this affects debugging?

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

Maybe just a matter of taste or semantic perspective but since it's literally a pass-thru calll I'd keep it a pass-thru call. Agree that materialising a hot enumerable can have side effects? Even it will happen anyway, and the source code is available, I would rely on the behavior of BCL/TPL rather than introduce possible side effects in MediatR. It's works very well as a 'glue' between application layers because it's lightweight. And I'd love to see it not to change.

This comment has been minimized.

@samueldjack

samueldjack Sep 2, 2016
Author

My philosophy was to keep existing code the way it was as much as possible: the original PublishAsync used ToArray() so I just preserved that.

Having said that, I think call stacks during debugging might be made a little easier to understand if the task is initiated within the WhenAll rather than before, so I'd have a preference for dropping ToArray too.

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

Both points are valid, sound good to me!

@@ -73,20 +88,65 @@ public void Publish(INotification notification)

public Task PublishAsync(IAsyncNotification notification)
{

This comment has been minimized.

@abatishchev

abatishchev Sep 1, 2016

To not introduce breaking change, I'd make this an overload having the default behavior.

This comment has been minimized.

@samueldjack

samueldjack Sep 2, 2016
Author

The problem with this is that I'd then have to update every call site in my code to get the behaviour I want - since I want sequential behaviour whenever events are published.

If the default option on the Mediator is configured correctly then there would be no breaking change in behaviour irrespective of how the method is called.

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

I see. This would be an issue for me as well, will have to update every call site. So settings behavior in one place is a good idea.

/// <summary>
/// Initializes a new instance of the <see cref="Mediator"/> class.
/// </summary>
/// <param name="singleInstanceFactory">The single instance factory.</param>
/// <param name="multiInstanceFactory">The multi instance factory.</param>
public Mediator(SingleInstanceFactory singleInstanceFactory, MultiInstanceFactory multiInstanceFactory)
public Mediator(SingleInstanceFactory singleInstanceFactory, MultiInstanceFactory multiInstanceFactory) :

This comment has been minimized.

@abatishchev

abatishchev Sep 1, 2016

Adding one more ctor is a breaking chance since some DI containers (such as Simple Injector) will stop working. So I'd keep only one and rely on the options parameters sent to an overload accepting it.

This comment has been minimized.

@samueldjack

samueldjack Sep 2, 2016
Author

I hadn't thought about the impact of adding an extra constructor on DI resolution.

Like I said in my comment on overloads of PublishAsync, I'd like to make this publish behaviour a global option so that I don't have to update every call site where events are published.

I've just checked out the breaking change rules for CoreFx and by my reading adding this extra constructor wouldn't be considered a breaking change since there's already a non-default constructor.

However, if the extra constructor issue is serious, perhaps we could have a property on Mediator to configure the default behaviour?

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

I'd rather go ahead with this change via ctor than add a property.
Either way it should be acknowledged/highlighted, and major version be bumped.
@jbogard your thoughts?

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 1, 2016

hey @samueldjack, thanks for converting my question to a pr!

@@ -21,17 +21,32 @@ public class Mediator : IMediator
private readonly ConcurrentDictionary<Type, Type> _genericHandlerCache;
private readonly ConcurrentDictionary<Type, Type> _wrapperHandlerCache;

private PublishAsyncOptions _publishOption;

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

Until a property is added, it can be made readonly.

/// <summary>
/// Initializes a new instance of the <see cref="Mediator"/> class.
/// </summary>
/// <param name="singleInstanceFactory">The single instance factory.</param>
/// <param name="multiInstanceFactory">The multi instance factory.</param>
public Mediator(SingleInstanceFactory singleInstanceFactory, MultiInstanceFactory multiInstanceFactory)
public Mediator(SingleInstanceFactory singleInstanceFactory, MultiInstanceFactory multiInstanceFactory) :
this(singleInstanceFactory, multiInstanceFactory, new MediatorConfiguration())

This comment has been minimized.

@abatishchev

abatishchev Sep 2, 2016

I'd make it more readable: new MediatorConfiguration { PublishAsyncOptions = PublishAsyncOptions.Parallel }, otherwise reader needs to go to the class's definition and learn the behavior from there

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 10, 2016

@jbogard any chance to review please?

@dotnetjunkie
Copy link

@dotnetjunkie dotnetjunkie commented Sep 10, 2016

@abatishchev

Adding one more ctor is a breaking chance since some DI containers (such as Simple Injector) will stop working

This shouldn't be a problem, since you shouldn't use your container's auto-wiring capabilities to create types of external libraries and frameworks, because of exactly this. Instead register external types using a delegate. This way you have full control over the constructor that is called.

Note that this is not a Simple Injector specific thing. This advice holds for all containers. Every container has its own unique way to select constructors.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 11, 2016

Hi Steven! I don't say it's a problem with SI or there are no workarounds. But if one already uses their container's auto-wiring capability than it will be a breaking change for them.
I'm not quite conveinced why one should not use it if it works and works flawlessly. In particular, MediatR has single ctor from the very beginning so why don't write Register<IMediator, Mediator>() rather than Register<IMediator>(() => new Mediator(...)) when you can? When you can't then you can't, and obviously have no other options. But until that it's nice to have such a choice.
That being said, is there workaround or other (and better) way to achieve the same? Yes. Would it be a breaking change? Yes. Would it be a big deal? No.

@dotnetjunkie
Copy link

@dotnetjunkie dotnetjunkie commented Sep 11, 2016

But if one already uses their container's auto-wiring capability than it will be a breaking change for them.

Be careful calling this a breaking change, because this would limit a reusable library builder like Jimmy to make changes to its library that would usually be stated as 'non-breaking' by The Framework Design Guidelines.

Adding overloaded constructors is typically the way for a framework builder to make these changes, because the signature of a constructor can't be changed (since that already is a breaking change).

I don't say it's a problem with SI

I would even say that this is less the problem with Simple Injector. In the case of Simple Injector you get a very clear exception message, which allows you to change your configuration. With a container that does constructor resolution, the framework builder never really knows which constructor will be selected (since each DI container has other rules for selecting constructors). And when you upgrade your application to the newest version of that external library, everything might keep compiling and no error is thrown when the application runs, but the application could still function incorrectly (but without warning or error), because the wrong constructor has be selected by your container.

If Jimmy follows the FDG (and I know he does), he should be able to add constructor overloads. If this breaks your application, it's not Jimmy's fault; it's yours.

so why don't write Register<IMediator, Mediator>() rather than Register(() => new Mediator(...)) when you can?

It's important to make the distinction between types you own and control and for which you know they have just a single constructor, and types of external libraries where you don't have any control over the number of constructors they have. In that case, my advice is to always call the constructor using C# code, not through reflection. Calling such constructor with normal C# code should not cause any maintenance problems, because their constructor signature will never change, compared to the types you control. We typically see constructor signatures change regularly while developing an application.

Of course I must admit here that MediatR is not your typical external library, so depending on how we see MediatR, the above description might not completely hold for MediatR.

When we follow the SOLID principles, they guide us to hiding external libraries from our application code; we create application-specific abstractions and create adapters to hide the external libraries behind. This however does not work with MediatR, since its goal is to provide you with the core abstractions to build your application around. So in this sense, MediatR must perhaps be seen as the central part of our application, otherwise we would be breaking SOLID big time.

On the other hand, MediatR is a reusable library used in hundreds (or perhaps even thousands) of applications and meant for general use. This immediately causes conflict with it being a central part of the application, since its changes and updates are out of control of the application, and its types must be treated differently than other application types (because of the possible additions of constructors for instance).

So whether or not you use auto-wiring to build up MediatR types depends on where you place MediatR in your application architecture, but do keep in mind that you don’t control MediatR.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 12, 2016

Indeed, these all make sense, even somewhere I could argue, but I'm a not good speaker and educator than you, so I have no other options than admit your rightness :)
The only thing I meant saying breaking change was not against such change but to bump major version. Right after

* pushed initiation of Tasks into the WhenAll handler
* set the default value of PublishAsyncOptions in the constructor for clarity
@jbogard
Copy link
Owner

@jbogard jbogard commented Sep 12, 2016

So....I don't like this for the main reason that it's very specific. I intentionally try to leave behavior out of the mix in the Mediator class, because it's opaque, you can't see what it does.

What I'd rather do is allow you to customize the behavior in any way you like, but not through a switch/if-then. Even if this is just a thing where we structure things so that you can subclass and override a virtual method, I'd rather do that than such a specific configuration here.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 12, 2016

So maybe for a separate behavior to have a separate method? Let PublishAsync() run in parallel and something else (e.g. PipelineAsync() or PublishSequnceAsync()) run sequentially?

But still passing an options parameter where one specifies the behavior is quite visible imo.

@jbogard
Copy link
Owner

@jbogard jbogard commented Sep 12, 2016

No - no options parameter. If something was going to be passed in, I'd rather it be a separate interface. INotificationPublisher or similar. No if/switch statement, make it a strategy pattern.

But overall, I'd rather subclass-and-override first. At least give people the option to add their own behaviors first before having a breaking change like this.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 12, 2016

Accepting INotificationPublisher makes sense. I'd ship MediatR with 2 implementations: Sequential and Parallel.

Or indeed subclass-and-override. Inherit Mediator by SequentialMediator and override PublishAsync, or decorate Mediator with SequentialMediatorDecoratory, proxy all methods but PublishAsync. Is this something you would ship with MediatR by default? If yes, I would submit a separate pull request.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 13, 2016

Please check this draft out: an PublishAsync() overload accepting IAsyncNotificationPublisher with its default implementation. Drawbacks: the implementation is internal since depends on internal wrapper class.

Here's another draft: a sequential decorator. Drawbacks: had to extract GetNotifications() method into few internal helpers to avoid code duplication.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 13, 2016

The task would be simplified once #67 is merged.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 13, 2016

Also if make cache static (and internal) as per #73 the task would be simplified even more.

@jbogard jbogard mentioned this pull request Sep 15, 2016
@abatishchev
Copy link

@abatishchev abatishchev commented Sep 21, 2016

I published one more draft: this time SequentialMediator inherits Mediator and overrides PublishAsync in a manner that no internal classes have to be made public.

@samueldjack
Copy link
Author

@samueldjack samueldjack commented Sep 22, 2016

Having thought about this further, I'm inclined to lean away from the sub-classing approach towards injecting a behaviour into the mediator. The biggest factor for me is that the subclassing approach immediately limits composability. It only affects one aspect of the Mediators behaviour (async behaviour) and yet it layers on top of the whole class.

Imagine another scenario where the sync behaviour needed to be customised and a subclass was introduced to support that. Then imagine wanting to support both the custom sync behaviour and the custom async behaviour. The only way to do that would be to reimplement the one behaviour as a further subclass of the other.

I guess the issue here is that the Mediator is a facade, pulling a couple of different kinds of behaviour (sync and async publishing) into one convenient interface. Thus the design needs to allow for independent variations in how the behaviours behind the facade are implemented - or split the behaviours as #102 is suggesting.

@abatishchev
Copy link

@abatishchev abatishchev commented Sep 27, 2016

@samueldjack sorry, missed to respond in time, I completely agree!

@jbogard
Copy link
Owner

@jbogard jbogard commented Dec 7, 2016

OK so this is all getting changed in 3.0. Stay tuned.

@jbogard jbogard closed this Dec 7, 2016
@abatishchev
Copy link

@abatishchev abatishchev commented Dec 7, 2016

Cool!

@jblackburn21
Copy link

@jblackburn21 jblackburn21 commented Jan 5, 2017

I can work on a PR to v3 if a direction is decided. This would now be need for async and cancellable async handlers.

Do you want to go the IAsyncNotificationPublisher route, and default to the parallel implementation?

@jbogard
Copy link
Owner

@jbogard jbogard commented Jan 5, 2017

Any direction that does not involve adding new constructor arguments. That'd be a breaking change and I'd need to go to 4.0.

@jbogard
Copy link
Owner

@jbogard jbogard commented Jan 5, 2017

That's why I suggested just a protected virtual method. It enables you to do whatever you want.

@codearoo
Copy link

@codearoo codearoo commented Feb 2, 2017

Hi... I was just looking at similar topics. I found where I can create a IAsyncNotificationHandler handler which is nice to allow the handler to decide if it should block the flow.. but is it in the works still to also allow the publishing code to determine if it cares if subsequent handlers are async or not? The Publish() method (using ver 3.0.0) is blocking until all non-async handlers finish.

@abatishchev
Copy link

@abatishchev abatishchev commented Feb 2, 2017

If you're looking for a fire-and-forget behavior you can decorate your handler with one that would enqueue on the thread pool inner handler's Handle() in one or another way, e.g.:

public FireAndForgetAsyncNotificationHandlerDecorator : IAsyncNotificationHandler<TNotification>
    where TNotification : INotification
{
    private readonly IAsyncNotificationHandler<TNotification> _inner;

    public FireAndForgetAsyncNotificationHandlerDecorator (IAsyncNotificationHandler<TNotification> inner)
    {
        _inner = inner;
    }

    public async Task Handle(TNotification notification)
    {
        await Task.Run(async () => await _inner.Handle(notification));
    }
}

(I don't guarantee this code will work as described but hope you got the idea)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.