Behavior components registration causing incomplete registrations

[jjanuszkiewicz]

This fails on NServiceBus 5.2.11 (with StructureMap 3.1.6.186):

var container = new Container();
var busConfig = new BusConfiguration();
busConfig.UseContainer<StructureMapBuilder>(c => c.ExistingContainer(container));
busConfig.UsePersistence<InMemoryPersistence>();
var bus = Bus.Create(busConfig);
container.AssertConfigurationIsValid();

The last line crashes with:

No default Instance is registered and cannot be automatically determined for type 'NServiceBus.Encryption.IEncryptionService'

There is no configuration specified for NServiceBus.Encryption.IEncryptionService

1.) new EncryptionMutator(*Default of IEncryptionService*, *Default of Conventions*)
2.) NServiceBus.Encryption.EncryptionMutator
3.) Instance of NServiceBus.Encryption.EncryptionMutator
4.) new DecryptBehavior(*Default of EncryptionMutator*)
5.) NServiceBus.DecryptBehavior
6.) Instance of NServiceBus.DecryptBehavior
7.) Lambda: IContext.GetInstance(value(StructureMapObjectBuilder+<>c__DisplayClass7).component)
8.) Instance of IBehavior<IncomingContext> (System.Object)

No default Instance is registered and cannot be automatically determined for type 'System.TimeSpan'

There is no configuration specified for System.TimeSpan

1.) new EstimatedTimeToSLABreachCalculator(*Default of TimeSpan*, *Default of PerformanceCounter*)
2.) NServiceBus.EstimatedTimeToSLABreachCalculator
3.) Instance of NServiceBus.EstimatedTimeToSLABreachCalculator
4.) new SLABehavior(*Default of EstimatedTimeToSLABreachCalculator*)
5.) NServiceBus.SLABehavior
6.) Instance of NServiceBus.SLABehavior
7.) Lambda: IContext.GetInstance(value(StructureMapObjectBuilder+<>c__DisplayClass7).component)
8.) Instance of IBehavior<IncomingContext> (System.Object)

No default Instance is registered and cannot be automatically determined for type 'System.Diagnostics.PerformanceCounter'

There is no configuration specified for System.Diagnostics.PerformanceCounter

1.) new CriticalTimeCalculator(*Default of PerformanceCounter*)
2.) NServiceBus.CriticalTimeCalculator
3.) Instance of NServiceBus.CriticalTimeCalculator
4.) new CriticalTimeBehavior(*Default of CriticalTimeCalculator*)
5.) NServiceBus.CriticalTimeBehavior
6.) Instance of NServiceBus.CriticalTimeBehavior
7.) Lambda: IContext.GetInstance(value(StructureMapObjectBuilder+<>c__DisplayClass7).component)
8.) Instance of IBehavior<IncomingContext> (System.Object)

The equivalent on NServiceBus 4.7.6 with StructureMap 2.6.4.1 succeeds:

var container = new Container();
Configure configure = Configure.With().StructureMapBuilder(container);
configure.InMemoryFaultManagement()
    .InMemorySagaPersister()
    .InMemorySubscriptionStorage()
    .UseInMemoryGatewayPersister()
    .UseInMemoryTimeoutPersister();
ConfigUnicastBus configUnicastBus = configure.UnicastBus();
IStartableBus startableBus = configUnicastBus.CreateBus();
IBus bus = startableBus.Start();
container.AssertConfigurationIsValid();
startableBus.Dispose();

[danielmarbach]

@jjanuszkiewicz Thanks for raising the issue. We will be looking into this. It seems this is coming from the following PR

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

// cc @justabitofcode @ramonsmits

[andreasohlund]

Probably the new interface that don't get registered https://github.com/Particular/NServiceBus/pull/3109/files#diff-6649827d4e8a8f8298fdee5cb143cd4d

[danielmarbach]

Ok, I think I see the problem. The new interface is not properly registered

https://github.com/Particular/NServiceBus/pull/3109/files#diff-e5926d2503e6f8c4074305c7992cb378R36

[andreasohlund]

Assigning to 5.2.15 since this needs patching

[danielmarbach]

Talked to @ramonsmits, he is going to fix it. Assigned him

[jjanuszkiewicz]

That was quick - thanks for looking into this! I realize it might be a bit early, but can you give any ETA for 5.2.15?

[ramonsmits]

@jjanuszkiewicz I'm trying to reproduce this issue.

The reason of this error is the validation of the registrations:

 container.AssertConfigurationIsValid();

You are not actually using any of the features where these entries are related to. I enabled all corresponding features and there was no exception.

busConfiguration.RijndaelEncryptionService();
busConfiguration.EnableSLAPerformanceCounter(TimeSpan.FromMinutes(1));
busConfiguration.EnableCriticalTimePerformanceCounter();

Probably locations that cause the issue:

• https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Encryption/Encryptor.cs#L73
• https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Monitoring/CriticalTime/CriticalTimeBehavior.cs#L34
• https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Monitoring/SLA/SLABehavior.cs#L33

The issue for these 3 entries is that we have incomplete registrations. Biggest question is, why are we enabling by default features that do not register all required dependancies and should be care? I think we do. I'm not familiar with how RegisterStep works as it seems that these behavior are registered somewhere in the object builder.

[ramonsmits]

@Scooletz helped to find how these behaviors are registered. We could not find (yet) how that happens with the register step logic. An existing contributor probably can answer this in a few minutes.

[danielmarbach]

@ramonsmits The important part is

https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Pipeline/RegisterStep.cs#L52 https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Pipeline/BehaviorChain.cs#L87

[ramonsmits]

@danielmarbach Yes, we saw those lines but that is when building the behavior pipeline. That should not contain any of these behaviors in the first place I think.

By explicitly disabling the features is seems the corresponding behaviors are still (partially) registered in the container.

busConfiguration.DisableFeature<NServiceBus.Features.Encryptor>();
busConfiguration.DisableFeature<NServiceBus.Features.SLAMonitoring>();
busConfiguration.DisableFeature<NServiceBus.Features.CriticalTimeMonitoring>();

Seems like the following is called even though the feature is not enabled although not entirely sure yet:

• https://github.com/Particular/NServiceBus/blob/master/src/NServiceBus.Core/Features/FeatureActivator.cs#L232

[andreasohlund]

Something sounds off, that line shouldn't be called if the feature is disabled?

[jjanuszkiewicz]

Is there any estimate when this will be fixed?

[ramonsmits]

@jjanuszkiewicz I'm continuing it now.

As a side-note, leaving out container.AssertConfigurationIsValid(); results in everything working but we are treating this as a bug as we should not have incomplete registrations.

Yes, there are incomplete registrations but they will not be resolved at all in this configurations as the dependent features are not activated (Encryptor, SLAMonitoring, CriticalTimeMonitoring).

I assume you want this to make sure all the dependencies for your handlers can be build by the container and that this is part of an acceptance test or similar?

[jjanuszkiewicz]

@ramonsmits Yes, this is part of a test procedure that we run on our components.

[ramonsmits]

@jjanuszkiewicz @andreasohlund Found the issue and resolved it in 9d4be09 and is part of PR #3518 which is now ready for review.

[ramonsmits]

Needs verification on V6 too.

[jjanuszkiewicz]

Guys, any update on this? And I can see that this is the single item in scope of 5.2.15, so will this actually get released?

[danielmarbach]

@jjanuszkiewicz Thanks for the ping. Please see the following statement from @timbussmann

• we decided to not change the way behaviors are registered as a hotfix in order to avoid breaking changes to the changed behavior.
• we don't see enough value in fixing it in a 5.3 release right now. Because it would still require us to provide a backward compatible mechanisms due to our release policy. While there are incomplete dependency hierarchies, these types will never be resolved and will not cause any harm or undesired behavior.

https://github.com/Particular/NServiceBus/pull/3518#issuecomment-192254707

[ramonsmits]

From: https://github.com/Particular/NServiceBus/pull/3518#issuecomment-191852473

Side-effects of AssertConfigurationIsValid

It seems that calling container.AssertConfigurationIsValid() has the side-effect of actually creating the types as commenting this statement started the bus as expected.

Inspecting the code doco:

Use with caution! Does a full environment test of the configuration of this container. Will try to create every configured instance and afterward calls any methods marked with the [ValidationMethod] attribute

Working solution / Workaround

The only working solution is:

Enable all features:

busConfiguration.RijndaelEncryptionService();
busConfiguration.EnableSLAPerformanceCounter(TimeSpan.FromMinutes(1));
busConfiguration.EnableCriticalTimePerformanceCounter();

Make sure you have atleast a dummy message with an encrypted property as not having this does not complete the encryption service setup. This also requires a valid encryption service configuration to satisfy the core IEncryptionService implementation but can we circumvented by passing a fake implementation.

busConfiguration.RegisterEncryptionService(b=>new FakeEncryptionService());
// or
// http://nsubstitute.github.io/
busConfiguration.RegisterEncryptionService(b=>Substitute.For<ICalculator>()); 

[jjanuszkiewicz]

@ramonsmits Thanks for the detailed follow-up.

While I understand the technical difficulties with this issue and I agree this is not critical (it only affects component "self-test" in our case), I'll really appreciate it if a fix is released in 5.3 (as I understand 5.2.x is not an option, right?)

We've just updated from 4.6 to 5.2 and given that 6 is still a work in progress, and already has a ton of changes, I don't see us adopting v6 anytime soon. Therefore I really hope this issue will be fixed in a 5.x minor update, as this is a regression against v4 and causes us to have a part of our self-test code disabled.

The proposed workaround is not acceptable - I'm not going to enable some random features and fiddle with configuration in production code just to make the selftests pass.

[timbussmann]

Hi @jjanuszkiewicz

We guarantee backward compatibility between minor versions. Changing the way we register behaviors would introduce a breaking change. This means we can only solve this as a new feature which allows to change the binding mechanics. While I won't say this won't happen in v5, our backlog is already quite full. There is no risk of data or message loss. It's not even affecting runtime behavior of the endpoint therefore this isn't classified as a critical bug.

AssertConfigurationIsValid() has side effects which we're not taking into account. So even if we fix the issue you're experiencing right now, there is no guarantee it will work afterwards or not break with future changes. It's not part of the NServiceBus Framework and we're not actively verifying compatibility with AssertConfigurationIsValid() or it's side effects.

We agree that the current behavior isn't optimal (regardless of AssertConfigurationIsValid()) and we're taking this into account for the v6 release (which allows us to introduce breaking changes). I can understand that the provided workaround is ugly and not very satisfying for you but this will most probably not change anytime soon.

[ramonsmits]

@jjanuszkiewicz You are not using this in production right? As mentioned, the side effects is that it actually creates all registrations meaning it will also start timers etc. and run tasks. You really don't want to just do that when an endpoint starts in production.

[jjanuszkiewicz]

Yes, I realize that, thanks. We run it rutinely as part of integration tests (so not on prod env), but also have a possibility to trigger it on demand on a running component on any env, including production - although this is not done automatically.

I'm wondering if we could maybe change the approach a bit, as the actual reason why we run AssertConfigurationIsValid() is to verify correct registration of our classes. So I'm thinking, maybe we should have a separate StructureMap container for NSB internals (or even let it go with whatever the default container is) and use the "main" SM container for our registrations only? But in that case, how can we register IBus and ISendOnlyBus in our container while leaving everything else in NSB-internal one? Would something like this be correct from NSB point of view (not yet tested if it works or even compiles):

var nsbContainer = new Container();
var busConfig = new BusConfiguration();
busConfig.UseContainer<StructureMapBuilder>(c => c.ExistingContainer(nsbContainer));
busConfig.UsePersistence<InMemoryPersistence>();
Bus.Create(busConfig);

var mainContainer = new Container(x =>
    For<IBus>().Use(ctx => nsbContainer.GetInstance<IBus>());
    For<ISendOnlyBus>().Use(ctx => nsbContainer.GetInstance<ISendOnlyBus>());
);
mainContainer.AssertConfigurationIsValid();

This should leave NSB internals in a separate container and just forward resolving of IBus and ISendOnlyBus to that container. AssertConfigurationIsValid() on the main container would not try to instantiate NSB internals. Would that be a recommended approach?

[timbussmann]

you could probably have a dedicated test which just creates your container and calls AssertConfigurationIsValid on it without including NServiceBus at all. In that case you only need to provide a fake binding for IBus before you validate the bindings. Would that work?

[jjanuszkiewicz]

That might work, yes. Thanks for the tip, I'll see what works best for our case.

[timbussmann]

@Particular/nservicebus-maintainers as long as we don't explicitly see AssertConfigurationIsValid as supported (and included in the testing) I don't think there is a way to actually "fix" this. As long we don't verify that behavior, there's a risk of introducing a registration on every hotfix version which can break AssertConfigurationIsValid again. So I'd say we either:

• Close this bug without fixing it as we can't guarantee it
• Officially support and test it (in that case let's raise a dedicated issue)

Given the ideas of reducing dependency on containers in general, option 2 seems to contradict this vision.

[danielmarbach]

We know that we should not register our own types on the user container and we definitely want to move away from that. It would probably take a long time to achieve this vision but eventually we will be getting there. I think this issue contains enough valuable and good design choices as "workarounds" to this problem like:

• Create a dedicated test which loads the binding modules for the types that contain the actual business logic and assert the configuration of that one or
• For send only instances create a dedicated container for the NSB stuff and forward the resolve of IBus and ISendOnlyBus to the dedicated container from the main container

So let's Close this bug without fixing it as we can't guarantee it

[timbussmann]

seems that we agree. Closing this then for the reasons in the comments before.