Umbraco-cms: Netcore: Remove static events

[rustybox]

There are 180 static events in the solution.

ContentService has 27 static events, they cause a world of pain both when building umbraco sites and when testing.

• Saving
• Saved
• Publishing
• Published
• ...

These can be replaced with an in process EventAggregator

We could have a simple one defined within Umbraco with an implementation similar to Udi Dahan - DomainEvents example

Or we could make use of MediatR, which could be referenced in Umbraco.Infrastructure with an Umbraco abstraction wrapping it.

Lots of discussion on this topic in #8653 and probably elsewhere.

---

If this work were to take place, IComponent is no longer required, most of the components just hook static events and could disappear.

Any existing components that must exec code on application startup e.g. ManifestWatcherComponent can be replaced by a handler for an ApplicationStarting event, this would have a RuntimeLevel property such that code could be conditionally run as required.

[Shazwazza]

Events are a mess 😭 There's a lot of legacy in there and a lot of things that should be killed or do strange things that shouldn't be done that way. It's gonna be quite an undertaking to get this all done but the amount of code and pain that will be removed will be very substantial. I'm not sure what the best approach to take here is going to be while minimizing the sweeping changes that will need to be done so the work can be done in smaller increments. Potentially one way would be to leave the events as-is currently and add the event aggregator code side-by-side then iteratively remove the old events with the new replaced handlers. Some things to know/keep in mind:

• The way Scopes (transaction / uow) work is to only let events raise once the trans is complete for the completed style events (suffixed with "ed" like "Saved") though these events are registered to be raised from within the services themselves. This code could be massively simplified without having to work around limitations of c# events.
• events suffixed with "ing" like "Saving" are raised during the transaction at the time they are raised in the service and in many cases are cancelable
• Some events exist at the repository level due to legacy and/or never getting around to fixing that but they shouldn't exist there at all
• Some events probably don't make any sense and could be removed but that would be a case by case assessment
• There are other static events that are not just on services that will need to be changes
  
  
  
  • One that needs to be changed is the BackOfficeUserManager. This has static events but the instance is a Scoped instance so it cannot have normal events. I suppose that makes it an ideal candidate for an event aggregator since we don't have to worry about binding to an instance.
  
  
• This work will require some patience

Fixing events would definitely make me very happy! But my concerns are

• The time it will take to change all events to use this new pattern since this is outside of the initial scope of the first netcore version release
• Breaking changes. Events are a pretty integral part of people's builds and would assume that a majority of Umbraco sites use events in one way or another so thinking about how much work it would be to transition to the new format from the old.
  
  
  
  • Can we maintain the same 'event args' members, etc... ?
  
  
  • And/Or would it be possible to have the service events remain but be obsoleted with event aggregator running side by side until the next major version after the first netcore version to remove them?
  
  

[lars-erik]

The way Scopes (transaction / uow) work is to only let events raise once the trans is complete for the completed style events (suffixed with "ed" like "Saved") though these events are registered to be raised from within the services themselves. This code could be massively simplified without having to work around limitations of c# events.

I'm not sure I'm in on the notion that "*ed" events should run after transactions? Do the transactions support nesting at least? I'm a huge fan of using request level transactions (and uow.commit) to allow "infinite" extensions of components involved in a use-case. That likely stems from most, if not every request, being either a "pure" command or a query. Those are of course decoratable, interceptable and composable. The API controllers / services in Umbraco are theoretically so as well, tho we have that big class vs. one class per use-case issue.

What if I don't want the document to be created after all if my post-event handler fails to do something after the doc. was created?

On a sidenote, there's also the issue of in-proc vs. out-of-proc for events to consider. Sooner or later when the architecture's shifted to events/commands/queries, events and commands will be processed out-of-proc. (We've already got several peeps sending a HttpRequest to threads. 🤓) I guess people will have to mind this themselves in all cases, but worth noting that out-of-proc handlers might be triggered within a transaction. IMO, that's an imperative to make event parameter classes immutable. (Records?) Otherwise peeps are bound to mutate parameters and possibly mess up assumptions that a part of the handler chain may depend on changes by another. (Also relevant wrt. transactions)

Can we maintain the same 'event args' members, etc... ?

As long as they're immutable. ;)

Typing this leaves me suspecting that lots of the events are passing entities _in order for_ them to be mutated. If we really really want this, could we mark or identify events that _must_ be handled in-proc?

Dunno if @JimBobSquarePants have any strong thoughts on this? (Or @rustybox?)
I preached a lot of this to @mattbrailsford for vendr as well, maybe he's got some insights to share. 👼

[rustybox]

Firing events on UOW complete comes up pretty regularly, just shove them in a collection and fire the lot when ready.

https://lostechies.com/jimmybogard/2014/05/13/a-better-domain-events-pattern/

---

Throwing stub event aggregator with whitelisted events example in here too for visibility over #9510

https://gist.github.com/rustybox/2eb3815b767edd9b5dc7bc751572cf17

[lars-erik]

Firing events on UOW complete comes up pretty regularly, just shove them in a collection and fire the lot when ready.

I was talking about:

Typically, I want the side effects of a domain event to occur within the same logical transaction, but not necessarily in the same scope of raising the domain event.

(from Bogard article)

Let's not mix transactions and uow. ;) When I say transaction, I mean the "scope", or rather a System.Transactions.Transaction.
I say it's better to pass the ID of the entity in the event messages having subsequent handlers fetch them again. Being within the same transaction, they can happily query and modify the entity as well, and then be handled by the same transaction conclusion. Provided they all run in-proc. I guess out-of-proc would process after the fact in any case, or at least that you'd have to delay processing in order for in-proc to complete.

[JimBobSquarePants]

Whenever I hear about work being "In scope" vs "Out of scope" development-wise I throw my hands up in exasperation. yes, these improvements make more work now, yes these improvements also save an absolute ton of work/pain in the future. If you give a developer two ways (good and bad) to do something they will use both ways and you will not ever be able to remove the bad one.

If a sensible development pattern had been adopted instead of trying to create an unholy union of legacy and new then we wouldn't have issues implementing correct patterns since there wouldn't be wading through decades worth of code to untangle it. The cost here is the legacy not implementing better patterns!

Regarding mutability. EF Core allows manipulation of entities during save, something to bear in mind.

[lars-erik]

f a sensible development pattern had been adopted instead of trying to create an unholy union of legacy and new then we wouldn't have issues implementing correct patterns since there wouldn't be wading through decades worth of code to untangle it.

Say it's exponential. If it started in year 2000 and increase exponentially every year, we're now at a mess scale of 10^20. Let's not make it 10^21, or worse 10^22. 😆

[rustybox]

What is mess scale? can we please use the industry standard WTF Per Minute

[lars-erik]

To make it realistic (Umbraco launched 2003?), let's say we're at a mess scale of 10^17 WTF per hour.
Notation: 10^17 wtfp/h.

[Shazwazza]

I'm not sure I'm in on the notion that "*ed" events should run after transactions?

This is the current behavior.

Do the transactions support nesting at least?

Yes this is the current behavior

What if I don't want the document to be created after all if my post-event handler fails to do something after the doc. was created?

Currently you can only cancel an event in the "*ing" events which occur during the transaction.

There are other weird events that are specific to nucache. I've had to investigate them yesterday with some changes I needed to do so have added a bunch of notes and docs to them. These events fire after the "*ing" events but during the transaction for the same purpose that you referred to: So that any DB changes nucache makes are done within the same transaction to guarantee data consistency.

Typing this leaves me suspecting that lots of the events are passing entities in order for them to be mutated. If we really really want this, could we mark or identify events that must be handled in-proc?

Currently this is the case yes. The entities are normal mutable entities like IContent which can be manipulated in "*ing" events, for example to populate default values. But in theory it is correct that these shouldn't be mutated in other events but that would be possible today.

Firing events on UOW complete comes up pretty regularly, just shove them in a collection and fire the lot when ready.

Yep this is the current behavior, was just mentioning this as something to note about events. Right now this is done with IEventDispatcher which will just need to be re-made and simplified since it won't need to deal with c# events and EventHandler instances. There's a lot of code related here that could just be killed i think.

Whenever I hear about work being "In scope" vs "Out of scope" development-wise I throw my hands up in exasperation. yes, these improvements make more work now, yes these improvements also save an absolute ton of work/pain in the future. If you give a developer two ways (good and bad) to do something they will use both ways and you will not ever be able to remove the bad one.

Re-phrased: To get this work done we need the community's help to do the work. If folks have the time to deliver this for first netcore release then that is awesome.

If you give a developer two ways (good and bad) to do something they will use both ways and you will not ever be able to remove the bad one.

So that answers a question - we should not ship with obsoleted old ways of doing things alongside new ways.

---

Moving forward, where/when/how do we start?

There's events all over the place, not just at the service level. If the first goal should be to remove static events then we can start small. IIRC the plan is still to remove all of the core components/composers and move them to IUmbracoBuilder extensions. Even while we still have the old event model this should be done and until events are totally changed to event aggregation only components/composers would be used by packages.

• PublishedRequest - 2x static events
• ServerVariablesParser - 1x static events
• EditorModelEventManager - 5x static events
• TreeControllerBase - 3x static events
• ContentModelBinder - 1x static events
• BackOfficeUserManager - 12x static events
• Cache refreshers - could probably be entirely refactored to use event aggregation. This could be a rabbit hole though and isn't critical 'now' but they do have one static event: CacheUpdated
• Service level events - the vast majority of static events

Thoughts?

[rustybox]

IIRC the plan is still to remove all of the core components/composers and move them to IUmbracoBuilder extensions.

I think the focus switched here from #9446 to avoid duplicating work, no need to move a component registration from a Composer if the component no longer exists as it has been replaced by a couple of calls to UmbracoBuilder.AddNotificationHandler

I'm planning on pitching in for the effort but lacking capacity until the end of next week.

[JimBobSquarePants]

IEventDispatcher shouldn't be a thing. Each service implementation should responsible for triggering its own events and should inject and use IEventAggregator for that purpose as and when required.

Events types don't need to inherit anything other that INotification. Being cancelable etc is an implementation detail of the individual event. How those events are then handled after firing is again an implementation detail of the service that triggered the event.

[Shazwazza]

Yep for sure that's all good. The IEventDispatcher is currently used to queue events which we'll still need to do in some way so events/notifications are triggered when the Scope is complete. IEventDispatcher can die and be replaced by whatever is simplest. Like @rustybox mentioned above

Firing events on UOW complete comes up pretty regularly, just shove them in a collection and fire the lot when ready. https://lostechies.com/jimmybogard/2014/05/13/a-better-domain-events-pattern/

---

I think the focus switched here from #9446 to avoid duplicating work

ah yes of course :) All good then I guess the gist is that we'll get there eventually one way or another. With the work being done on the front-end routing poc there will be a few less components already

[rustybox]

With the work being done on the front-end routing poc

Where can that be tracked? really exciting stuff, can't wait to deploy a pre-release on a cheap linode/droplet

How those events are then handled after firing is again an implementation detail of the service that triggered the event.

~Pub/Sub, presumably you meant implementation detail of the subscriber, publisher cares not~
you meant how cancelled events are handled by the publisher, gotya

[rustybox]

QueuingEventDispatcherBase.cs

this is way too convoluted, the supersede attribute is used only on DeleteEventargs to specify
that it supersedes save, publish, move and copy - BUT - publish event args is also used for
unpublishing and should NOT be superseded - so really it should not be managed at event args
level but at event level

what we want is:
if an entity is deleted, then all Saved, Moved, Copied, Published events prior to this should
not trigger for the entity - and even though, does it make any sense? making a copy of an entity
should ... trigger?

not going to refactor it all - we probably want to always trigger event but tell people that
due to scopes, they should not expected eg a saved entity to still be around - however, now,
going to write a ugly condition to deal with U4-10764

Is any of this important, how does one get a delete and a save in the same scope?

Phrased another way, do we need to have a OrderedHashSet\

I can only imagine someone wrote all this code for a reason, but it's not obvious to me the problems that are being solved.

Edit:

https://issues.umbraco.org/issue/U4-10764

scope and events are somewhat broken, never realised it

:)

[Shazwazza]

oh boy this brings back some vague memories

the notes referenceThis issue sheds some light on some of this: https://issues.umbraco.org/issue/U4-10764

Is any of this important, how does one get a delete and a save in the same scope?

A scope can wrap any number of service calls so you can definitely add, delete, modify, etc... anything within the same scope - think of a package installation. Like the note in the comments says "if an entity is deleted, then all Saved, Moved, Copied, Published events prior to this should not trigger for the entity". I cannot recall the specific circumstance that the same entity will be saved and deleted in the same scope but it happens.

IIRC the reason why a Delete event would 'supercede' a previous event like saving, etc... is because things subscribing to save, etc... events shouldn't actually be notified of that save operation if it's actually just been deleted. For example, in Examine, there's no point in indexing an item just to delete it a moment later - similarly with nucache. The end result is the entity doesn't exist so anyone listening for events only needs to know about that one.

[Shazwazza]

@rustybox for front-end stuff the current work is here https://github.com/umbraco/Umbraco-CMS/pull/9530

[rustybox]

How do I find the notes section on issues.umbraco or is that synonymous with comments section etc?

I cannot recall the specific circumstance that the same entity will be saved and deleted in the same scope but it happens.

Presumably though we could simplify the setup and just recommend that people create multiple scopes when doing weird stuff like that.

in Examine, there's no point in indexing an item just to delete it a moment later

Was this profiled as an issue that mandated the complexity for performance gains or just premature optimization?

[JimBobSquarePants]

This all reads like we’re attempting to fix an issue that simply shouldn’t occur. My advice.... Find out what EF do and copy it.

[rustybox]

I don't think EF team worry about superfluous event handling for examine and nucache 😄

IMHO it's fine to just fire all the events that occurred, there's already a raiseEvents flag on content service methods if one wants to skip them for whatever reason.

[JimBobSquarePants]

What EF do is fire a single save event with all the detail in that message. You have ChangeTracker.Entitites which you query for deleted, updated, added. Attempting to track then fire individual events for an entities state is noise IMO.

[Shazwazza]

How do I find the notes section on issues.umbraco or is that synonymous with comments section etc?

Sorry that was just some borked typing of mine, have fixed up. I just meant the issue and comments have some info there that might be slightly helpful

Was this profiled as an issue that mandated the complexity for performance gains or just premature optimization?

I can't recall unfortunately but there was both de-duplicating events along with superseding events so there wasn't unnecessary processing in events. I really don't remember though if that stemmed from benchmarked analysis or not.

there's already a raiseEvents flag on content service methods if one wants to skip them for whatever reason.

I would like to kill this if possible. I would prefer being able to tell the Scope to not raise events instead of individual methods on the service.

I'm pretty sure there's Umbraco Deploy considerations in here too because it does a lot of bulk processing with the services and it suppresses and/or collects the events differently so that performance is ok. I can't remember off the top of my head how that works or what it does but that is definitely benchmarked performance.

Find out what EF do and copy it.

AFAIK it's just simple events on the DBContext (in our case Scope). I could be wrong but I don't think they have events that are raised just before the trans is completed (like this conversation: https://github.com/umbraco/Umbraco-CMS/pull/9530#discussion_r540994463)

In our services we currently send events via the Scope so the scope has control of how events are raised which I think is fine. Just needs to be overhauled :P

[JimBobSquarePants]

overhauled deleted. FTFY.

Those simple events should be enough IMO.