Particular / NServiceBus

Build, version, and monitor better microservices with the most powerful service platform for .NET
https://particular.net/nservicebus/
Other
2.07k stars 650 forks source link

Add explicit handler assembly registration mode instead of relying on scanning all assemblies #5595

Open danielmarbach opened 4 years ago

danielmarbach commented 4 years ago

Personally I think that the whole assembly process is backwards. Why not just have a AddHandlerAssembly method in the assembly scanner (which means that no assemblies should be automatically added). Most devs know which assemblies to include, but it’s harder to understand which ones must be exluded. That also improves the startup time since everyone that wants to exclude assemblies must scan through all assemblies (or files) to be able to exclude everything that doesn’t contain handlers, while for AddHandlerAssembly no scanning is required at all (and you typically want to include a lot less assemblies than you must exclude, especially in .NET Core projects).

Suggestion from Jonas in https://discuss.particular.net/t/request-reply-that-hangs/1622/3

dvdstelt commented 4 years ago

We have ExcludeAssemblies at the moment: https://docs.particular.net/nservicebus/hosting/assembly-scanning#exclude-a-list-approach-exclude-specific-assemblies-by-name But we used to have IncludeAssmblies. Maybe bring that back?! :-)

jgauffin commented 4 years ago

Let's take an .NET core project for example. A minimal ASP.NET Core project contains about 70 assemblies.

Typically only a few of them will container handlers, which means that with the Exclude approach you need to scan through all 70 assemblies and invoke the Exclude method for 68 of them.

If you have a IncludeHandlerAssembly method you only need to invoke it twice (for the example above).

What I suggest is this:

Add a AddHandlerAssembly method (please not just AddAssembly as that is ambiguous).

If AddHandlerAssembly was invoked:

  1. Add system assemblies (like Core and CallBacks if referenced)
  2. Scan specified assemblies for handlers.

If AddHandlerAssembly was not invoked:

  1. Use same handling as today.

And let the documentation list which assemblies must be included (today it just says Core).

dvdstelt commented 4 years ago

Hey @jgauffin, thanks for the feedback so far.

We do more than add (and scan) assemblies for handlers though. Also sagas, messages and more. Do you think endpointConfiguration.AssemblyScanner().AddAssembly(..) or endpointConfiguration.AssemblyScanner().IncludeAssemblies(..) would still be ambiguous and/or not clear enough or need more refinement?

jgauffin commented 4 years ago

The documentation states:

During the scanning process, the core assembly for NServiceBus (NServiceBus.Core.dll) is automatically included since it is required for endpoints to properly function

It also states:

NServiceBus extensions such as NServiceBus.RavenDB.dll are not considered a core assembly but will still need to be included when customizing the assembly scanning.

That in combination of a method named AddAssembly gives you no direction at all about which assemblies must be included. You either have to make an educated guess or try until you get it right.

Why not either just state something like "All NSB assemblies must be included plus your handler and saga assemblies".

In my case I did an educated guess. And as a result the callbacks stopped working (NSB gave me the "No handler can be found") which made it hard to back track it back to the assembly loading optimization.

And I assume that the kind of error will differ depending on which function you use and which assembly you forget to include.

The documentation also states:

To limit the number of assemblies being scanned and hence provide improvements to startup time

and

An "Exclude a list" approach is used. This supports that the common scenario removing specific assemblies from scanning without the common side effect of accidentally excluding required assemblies.

Well, to improve startup time you want to remove as many assemblies as possible, which means that you will accidentally remove required assemblies ;) No sane developer will manually exclude all .NET core assemblies.

bording commented 4 years ago

I think the larger issue here is why you're wanting to exclude assemblies in the first place. The current design is intended for you to not need to do anything, and it just works. The assembly exclusion API is intended to be an escape hatch for a problem, or if you're doing something more advanced like having more than one endpoint hosted in a single process.

If the reason you're trying to exclude assemblies is because of startup performance, then I think we should be focusing on why you're seeing bad performance, and fixing that.

@jgauffin What sort of startup times are you seeing that has made you start using the exclusion API? I know you've also got a discussion thread about this, and that's why I asked to see more information from when you were profiling it. I'd like to see if there's a way for us to improve the scanning performance.

jgauffin commented 4 years ago

The thing is, you have the API, If it should not be used, deprecate it. If the purpose is not to improve startup performance remove that statement from the documentation.

I'll do measurements tomorrow, with and without my assembly exclusion.

jgauffin commented 4 years ago

I've updated the discuss thread.

Nine seconds is a quite large difference.

danielmarbach commented 4 years ago

I did a quick spike that excludes in a rather blunt and brutforce way the system and microsoft assemblies

https://github.com/Particular/NServiceBus/pull/5596

It cut down the scanning time in half

caleb3141 commented 3 years ago

We're currently on NServiceBus 4.x trying to upgrade to 7.x and running to the issue where we previously ran Configure.With(assembly1, assembly2, assembly3) to explicitly opt-in to scanning various assemblies.

This method is much preferred as it's very explicit to what you're pulling in vs. running into a gotcha where a class will be registered automatically and cause unexpected issues.

isaacabraham commented 3 years ago

Just my two cents on this - I was playing around with NSB in F# and - as always when trying out a new library for the first time - I try it from a script just to get a feel for the library.

In this case, I struggled quite a bit because of the assembly scanning as the only real way of locating Message Handlers. I worked around it using the ExecuteTheseHandlersFirst method, but something that is easy, flexible and basically acts as an escape hatch is to simply has a method that takes in a list of IHandleMessages types.

Going another step further would be to turn over control of the instantiation of the values to the caller, so a list of Func<IHandleMessages>. This is particularly useful in F# where you don't need to define formal interfaces to implement them - you can simply new up an instance of an interface and provide implementations inline. Providing this would allow a much more function-first approach.

One or both of these would make the product more attractive to F# works (and lowering the barrier to entry).