Nservicebus: Subscriptions should respect specific type

[SimonCropp]

repro https://github.com/Particular/BugsRepro/tree/master/1942SubscriptionsShouldRespectSpecificType

so I have a mapping in the subscriber

<add Assembly="Publisher.Messages" Type="Publisher.Messages.OneEvent" Endpoint="Publisher" />

Note I am explicitly saying I am interested in OneEvent.

And in the subscriber I have a handler

public class EventHandler : IHandleMessages<IMyEvent>
{
    public IBus Bus { get; set; }

    public void Handle(IMyEvent message)
    {
        Debug.Assert(message is OneEvent);
    }
}

In the publisher I do this

        Bus.Publish(new OneEvent());
        Bus.Publish(new TwoEvent());

Now because the subscriber has said in the mapping "i am interested in OneEvent" then it should only receive OneEvent. However it receives both OneEvent and TwoEvent

So the logic we currently have for subscriptions is

• look through the mappings
• for any of the types check if we have handlers for the base types
• subscribe to the base types

This has unintended side effects as I have shown above.

To further illustrate how this is wrong a user should not expect these 3 mappings to behave the same way

<add Assembly="Publisher.Messages" Type="Publisher.Messages.TwoEvent" Endpoint="Publisher" />

<add Assembly="Publisher.Messages" Type="Publisher.Messages.OneEvent" Endpoint="Publisher" />

<add Assembly="Publisher.Messages" Type="Publisher.Messages.IMyEvent" Endpoint="Publisher" />

If we are interpreting both TwoEvent and OneEvent to mean IMyEvent we should throw an exception in the first two cases to avoid allowing the user to ambiguous+misleading configuration

[johnsimons]

Note I am explicitly saying I am interested in OneEvent.

So why is your handler handling IHandleMessages<IMyEvent>, shouldn't it be IHandleMessages<OneEvent> ?

[SimonCropp]

@johnsimons that may be the general case. But we should not be dictating where in the hierarchy chain of a type a user has a handler for. My point is handlers should only be used to validate the subscription mappings not to change them.

So I map to OneEvent, as long as I have at least one handler that can handle it (in this case IHandleMessages<IMyEvent>) then my mapping for OneEvent is valid. This is not the same as changing my mapping to be for IMyEvent instead of OneEvent

[johnsimons]

But you are using polymorphic messages.
So by stating that you are interested in OneEvent, you are in essence saying that you are interested in OneEvent + any other base classes or interfaces OneEvent derives from or implements, in this case IMyEvent.
Now whether you want to consume IMyEvent or any other base types is up to you.

In your case you are saying that you want to consume IMyEvent by having a handler for it, so the framework is sending a subscription request to the publisher for IMyEvent.
If you are not interested in IMyEvent then do not have a handler for it and instead just have a handler for OneEvent

Will this work for you?

[SimonCropp]

from our doco http://docs.particular.net/NServiceBus/how-to-pub-sub-with-NServiceBus

<!-- To register a specific type in an assembly: -->
<add Assembly="assembly" Type="type fullname" Endpoint="queue@machinename" />

I am doing just that. I am registering my interest in a specific type . The doco makes no mention of what handlers I have. Handlers are for how I want to handle messages not how I want them routed.

[SimonCropp]

I could understand the current behavior if I was using config like this

<add Assembly="assembly" Endpoint="queue@machinename" />

Not the lack of a type

Then it makes sense to infer from the handlers

[SimonCropp]

also i think the phrase using polymorphic messages does not mesh very well with what our doco states i am doing... register a specific type.

[andreasohlund]

I think this is a bug, if you subscribe to OneEvent you shouldn't get the sibling TwoEvent just because they share base classed @udidahan ?

[johnsimons]

If this is a bug then this bug exists in v3 and quite possible v2

[udidahan]

I think the question is "how does one express their intent to subscribe?".

If it is via the handler, then I'm not so sure this is a bug - or more accurately, an exception should have been thrown (in the case that only OneEvent was in the config) indicating that we have incomplete routing information.

If it is via the config, then it probably is a bug.

Given that the config isn't necessary for broker transports, I'd say our stance should be the former - subscription intent is based on the handler.

In any case, our documentation needs to be clearer here.

[johnsimons]

So based on this comment are we all happy to close this as by design ?

[SimonCropp]

@johnsimons

from the comment you pointed to

If it is via the config, then it probably is a bug.

And as per my description I am using config to define my subscription, so this is a bug.

[johnsimons]

Sorry @SimonCropp I should have been more specific.
As @udidahan points out in his comment, brokers do not require mappings, so because of that it makes sense to infer subscription intent based on the handler.

Does that make sense now @SimonCropp ?

[SimonCropp]

brokers do not require mappings

yep ok. but that is not the context of this bug. mapping is being explicitly configured and ignored.

[johnsimons]

The mapping is still being used because without it we would have not subscribed to any message at all, but because the most specific IHandle implementation is for the base type, we use that one.

[SimonCropp]

@johnsimons no matter what way you look at this "inferring from the handler" and "respecting the config" conflict. if you say "the handler should win" then we should throw an exception for the incompat config

[johnsimons]

I think as @udidahan points out, we just need to ensure our doco correctly explains how the combination of mapping and IHandle works.

Maybe we can log a warning that states that there was no specific IHandle for the specified type in the mapping ? And that would also cover situations where the mapping is present but no IHandler at all.

Do you think that is good way forward?

[johnsimons]

@SimonCropp can you please move this one to requirements and label it as a concern, it doesn't look like we reached an agreement so it makes sense to flush it out over there.

[danielmarbach]

@SimonCropp any ideas how to move this forward?

[SimonCropp]

@danielmarbach nope.

[danielmarbach]

Should we close this or hash out a strategy to address it?

[SzymonPobiega]

So the way the new routing taps into the old config (MessageEndpointMappings), is similar to V5 behavior: if you configure a route for type X, it will be also configured for all known message types that X derives from (including IMyEvent in this case).

Then, as previously, auto subscribe will subscribe based on the handlers discovered (IMyEvent).

IMO the best thing we can do is document the fact that when you specify a MEM for type X, it includes all types it derives from. Question here would be, what if EventOne and EventTwo have different publishers? If I specify both and have a handler for IMyEvent, where the subscribe message goes?

In the new Publishers API there is no inheritence magic so when you do busConfig.Publishers().AddStatic(typeof(EventOne), "SomeEndpoint") it won't register a route for IMyEvent.

[udidahan]

These are really edge cases - not a priority right now.

[SzymonPobiega]

@udidahan if the fix is just a doco clarification, I'd say it is worth doing.

[timbussmann]

@andreasohlund and I agree that this is a bug.

Possible fixes:

• Throw on "suspicious" mappings (mapping and handler mismatch): While this works for the sample given by simon, it doesn't work when there are three events deriving from the same interface and you you're only subscribing to the first two events. Then it's a perfectly valid use case to have a IHandleMessages<IMyEvent>().
• Subscriber should not receive sibling messages. E.g. when subscribing to OneEvent it shouldn't receive TwoEvent but when subscribing to IMyEvent it should receive both.

[timbussmann]

so with the new routing behavior the sample from @SimonCropp would behavior as following:

• When using autosubscription: the handler would receive no events, because there is no publisher configured for IMyEvent.
  
  
  
  • If there would be a route for IMyEvent, it would receive both OneEvent and TwoEvent.
  
  
  • If there is an additional handler for OneEvent, the endpoint would only receive OneEvent.
  
  
• When using manual subscriptions. e.g. endpoint.Subscribe<OneEvent>(), it would only receive OneEvent.

@Particular/nservicebus-maintainers @SimonCropp I think we can close this?

[SimonCropp]

@timbussmann

the handler would receive no events

isnt that a serious breaking change?

[timbussmann]

it is a breaking change, yes. We discussed a lot about routing inheritance (e.g. that IMyEvent can "inherit" the routes from OneEvent) and we decided to stop doing that in favour of a clearer rule about message routing. All current routing scenarios are still possible but may require additional routing configuration. Since this only affects very specific edge cases like described here (you're interested in OneEvent, but only have a hander for IMyEvent) and scenarios with multiple publishers for events which belong to the same message hierarchy, we agreed that this breaking change is acceptable. See the full discussion and description of the routing rules here: https://github.com/Particular/NServiceBus/issues/3911 (this comment should summarize most of the behavior: https://github.com/Particular/NServiceBus/issues/3911#issuecomment-235274881).

[SimonCropp]

so given if someone using v5 has the setup i mentioned the new v6 behavior would be message loss?

[timbussmann]

since autosubscribe doesn't have a route for IMyEvent, yes. /cc @SzymonPobiega (this seems to the argument for the proposed v5 backward compat?) @bording @DavidBoike

[andreasohlund]

I'm with @SimonCropp this new behavior, albeit for a corner case, would practically mean message loss since if you update to v6 you would no longer get the events you would get in v5?

[SzymonPobiega]

I think my last couple of comments on #3911 are aligned with what @SimonCropp said. TL;DR

• Yes, it is a message loss if somebody did not follow the upgrade guide carefully
• When looking at MEMs as description of message ownership it makes sense to "inherit" the routes both up and down the type inheritance chain
• For a given message type hierarchy, it should (ideally) be allowed to map only a single level i.e. either IMyEvent or (OneEvent and TwoEvent)

[andreasohlund]

Yes, it is a message loss if somebody did not follow the upgrade guide carefully

Unfortunately I don't think that is acceptable, me and @danielmarbach had the same approach to the removed .retries queue but after talking to @udidahan we came to realize that it isn't ok to require users to read doco in order to avoid message loss and the decision was made to reintroduce a empty satellite for this version. I can't see how this is any different? (and I think more likely to happen compared to unclean shutdowns leaving messages in the .retries queue)

[timbussmann]

Yeah, although I think the scenario is incorrect in the first place, if that is supported in v5 the upgrade story must avoid message loss scenarios.

[SimonCropp]

If this should not be supported... would it be viable to throw on the subscriber if there is no exact handler when we subscribe in the mapping?

[timbussmann]

If this should not be supported... would it be viable to throw on the subscriber if there is no exact handler when we subscribe in the mapping?

That may be worth investing whether to would work for all supported scenarios. The benefit with that approach would be that MEM and code first routing APIs would share the same behavior. /cc @DavidBoike @bording

could that check even be limited to autosubscriptions?

[SzymonPobiega]

Let's move this disucussion over to https://github.com/Particular/NServiceBus/issues/3911 because we are discussing the same thing in two places.

[timbussmann]

so if I understood correctly, v6 contains the same behavior/bug as stated in the initial description again for MEM (while the code first routing api has the behavior described in https://github.com/Particular/NServiceBus/issues/1942#issuecomment-240695629 ). Should we close this as won't fix then if the goal is to get rid of MEM in v7? @SzymonPobiega @SimonCropp @Particular/nservicebus-maintainers

[SzymonPobiega]

After talking with @andreasohlund in the routing TF we decided to keep the V6 handling of message endpoint mappings as close to V5 as possible to minimize the risk of message loss or other problems. Since MEMs are scheduled to be removed in future and the fix for this issue is not trivial I propose to close this one with won't fix. @SimonCropp ?

[danielmarbach]

Closing this with Won't Fix based on https://github.com/Particular/NServiceBus/issues/1942#issuecomment-244848397