Remove all behavior registration methods that take `Type`

[SzymonPobiega]

In V6 we changed the pipeline bootstrap behavior so that all the behavior instances are crated on start up rather than per-message.

Unfortunately we still have behavior registration methods such as:

public void Replace(string stepId, Type newBehavior, string description = null)

which take a type (or are generic). The way they work is they resolve the instance from the container only once (on start up) so they effectively ignore any life cycle settings users defined for that particular container registration. Given code like:

config.RegisterComponents(r => r.ConfigureComponent<MyBehavior>(), DependencyLifecycle.InstancePerCall);
config.Pipeline.Register(typeof(MyBehavior), "My fancy behavior");

the user expects his behvior to be instanciated for each message processed but in fact what happens is MyBehavior is created only once.

I propose we remove all the registration methods that are based on the type and leave only these based on a factory method or an existing instance.

[SzymonPobiega]

@Particular/nservicebus-maintainers please take a look. It is related to this doco pull and the comments by @weralabaj . TL;DR there is no way to unambiguously document how this works now.

[timbussmann]

but the factory also is only invoked once per pipeline if I remember correctly, so that would actually match the behavior of the type based overload? The type based overload has a good chance to get removed anyway when we remove the internal container usage, so not generally against it, but I'm not sure whether the argumentation here is really valid (since the factory overload also could lead you to the impression to have a per-message instance)? The "behaviors are created once per pipeline and are cached" documentation seems to be ok for me.

[weralabaj]

it's ok assuming that everybody will read the doco before just using that method. if not then you're up for a really nasty investigation.

when we remove the internal container usage

when is that planned for?

[SzymonPobiega]

@timbussmann the problem (which we have one documented occurrence on GG) is that if you just upgrade from V5 to V6, it continues to work but the behaviors are no longer per-call. No compile errors that would tell you to change code. That's why I strongly suggest removing the container-dependency type-based APIs. They simply don't work as they promise. I see no reason to keep them if we can't properly document them.

[timbussmann]

when is that planned for?

some future releases :laughing:

@SzymonPobiega let's play through the upgrade story if you had this code in v5 then:

config.RegisterComponents(r => r.ConfigureComponent<MyBehavior>(), DependencyLifecycle.InstancePerCall);
config.Pipeline.Register(typeof(MyBehavior), "My fancy behavior");

if we remove this overload you would just change your code to

config.RegisterComponents(r => r.ConfigureComponent<MyBehavior>(), DependencyLifecycle.InstancePerCall);
config.Pipeline.Register(builder => builder.Build<MyBehavior>(), "My fancy behavior");

and you would still make the same assumptions.

I don't think this makes it much better. It removes one source of potential misunderstanding and just drives you to another API with the exact same issue?

[SzymonPobiega]

@timbussmann when you update package to V6 line

config.Pipeline.Register(typeof(MyBehavior), "My fancy behavior");

will show as error with a proper obsolete message to use the factory method. Assuming people read error messages (I know that can be successfully challenged ;-) one might expect that a proper factory method will be used. But nothing is bulletproof.

[tmasternak]

@SzymonPobiega @timbussmann can you try to arrive at decision on this one? I think that core maintainers have the best context to decide if that should be part of V6 or that it can be postponed.

[bording]

It's hard to capture a behavior change to an API when nothing about the API surface has changed. In this case, anything other than passing in an already created instance is going to be hard to reason about when and how often we're going to create an instance. This is true when accepting a type or a factory method.

Given that we have actually changed this behavior from V5 to V6, the only way I can see that we can make this obvious without requiring consulting the documentation would be to actually change the name of the API in some way. For example, if we renamed it to something like RegisterBehaviorToBeCreatedOnceAndCached, then it would be much more obvious as to what to expect here. Of course, I'm not suggesting that exact name be used, but perhaps something less unwieldy that still gets the point across.

One point in favor to obsoleting the type overload is that we could then include something in the obsolete message that points out the behavior change.

[SimonCropp]

so this didnt make it for beta 6?

[timbussmann]

so this didnt make it for beta 6?

no.

[SimonCropp]

@timbussmann so is this still scheduled for v6?

[SzymonPobiega]

I believe it should be but it is up to maintainers to decide. Anyway, the decision has to be made because https://github.com/Particular/docs.particular.net/pull/1606 is blocked. If the API won't be removed we need to find a way to explain it properly. I guess the best approach would be for somebody who want's to keep the Type based API to provide a doco explanation for this feature.

[SzymonPobiega]

@Particular/nservicebus-maintainers ping

[danielmarbach]

@SzymonPobiega organize a quick call with @Particular/nservicebus-maintainers so that we can hash it out

[SzymonPobiega]

I don't see a reason to jump on a call. It seems a trivial issue to me. Either somebody from the maintainers group can provide a good doco on how the API works or we should remove the API.

[andreasohlund]

@SzymonPobiega Not against removing the type overload but it doesn't solve the issue as long as we have the public void Register<T>(string stepId, Func<IBuilder, T> factoryMethod, string description) overload. That one is still misleading since no matter what lifecyle the user configures his behavior as we will resolve it once and then cache it => make it it a "singleton" no matter what.

So regardless if we remove the type overload we need to document this behavior change properly in the upgrade guide.

One argument for killing the type based overloads would be that it gives us a chance to use the obsolete message to make users aware of the lifecycle change.

Also given that this means reducing our api surface area I'm :+1: for the change (talked to @SimonCropp and @timbussmann about this)

[SzymonPobiega]

@andreasohlund I think

public void Register<T>(string stepId, Func<IBuilder, T> factoryMethod, string description)

is a completely different story. It is easy to document and intuitive that the provided func is called once. What is misleading is code like this:

config.RegisterComponents(r => r.ConfigureComponent<MyBehavior>(), DependencyLifecycle.InstancePerCall);
config.Pipeline.Register(typeof(MyBehavior), "My fancy behavior");

which is clearly lying to the user.

[andreasohlund]

My point was that the IBuilder overload leads user towards registering things in the container which pushed users to the misleading code you mention?

So us removing the type one might even push more users towards it?

[danielmarbach]

See also issue https://github.com/Particular/NServiceBus/issues/4034 and PR https://github.com/Particular/NServiceBus/pull/4035

[SzymonPobiega]

@andreasohlund I don't agree.

With the Type API users had to register bahavior's dependencies in the container With the Func API users can, but don't have to register behavior's dependencies in the container The Func API does not say anything about the lifetime scope of the API so we can state clearly in its XML comments that the Func will be called exactly once per pipe If somebody has chosen to continue using container for behavior dependencies, it will still be correct even if he registers itself with PerCall. E.g. if in your func you decide to inject IFoo twice, you'll get two different instances.

[SzymonPobiega]

Thanks a lot @danielmarbach . You're the man!

[danielmarbach]

@SzymonPobiega :beer: and :cocktail: always welcome ;)

[andreasohlund]

@SzymonPobiega how relevant is this for v7?

[SzymonPobiega]

@andreasohlund hmm I am not sure. I don't have the V7 context.

[andreasohlund]

@danielmarbach thoughts on this one?

[danielmarbach]

I think we can postpone the decision on this one until we know more about the roadmap to removing the container.