Added option to enable PublishAsync to invoke handlers sequentially

[samueldjack]

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

[abatishchev]

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.

[khellang]

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.

[abatishchev]

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.

[khellang]

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?

[abatishchev]

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.

[samueldjack]

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.

[abatishchev]

Both points are valid, sound good to me!

[abatishchev]

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

[abatishchev]

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

[abatishchev]

@jbogard any chance to review please?

[dotnetjunkie]

@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]

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 then it will be a breaking change for them.
I'm not quite convinced 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 otherwise - 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]

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]

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

[jbogard]

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]

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]

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]

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]

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]

The task would be simplified once #67 is merged.

[abatishchev]

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

[abatishchev]

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]

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]

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

[jbogard]

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

[abatishchev]

Cool!

[jblackburn21]

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]

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.