Nservicebus: V6 - Prevent users from injecting IEndpointInstance and using it inside of Message handlers to send / publish messages

[indualagarsamy]

Relates to:
https://github.com/Particular/V6Launch/issues/27
https://github.com/Particular/PlatformDevelopment/issues/819

Problem

Often times customers have old / common libraries where a IBus was injected into the library. And the IBus was used to send messages / publish events from a self host or a NServiceBus.Host either in bootstrap scenarios or in message handlers.

However, IEndpointInstance/MessageSession is not safe to use from message handlers.
Some example of issues with sending messages using IEndpointInstance/IMessageSession within a message handler are:

• those messages will not participate in the same transaction scope as that of the message handler. This could result in messages dispatched or events published via the IEndpointInstance interface even if the message handler resulted in an exception and the operation was rolled back.
• those messages will not be part of the batching operation and therefore will not have the benefits that Version 6 provides.
• those messages will not contain any important message header information that is available via the IHandlerMessageContext interface parameter, e.g., CorrelationId.

How can we stop new and existing users from writing "invalid" code from either compiling or throwing a run time warning / exception?

Because without it and just relying on guidance, users can still run into issues with code that compile and seems like it runs fine on the surface while leading to undesirable run time downsides, especially when a message handler transaction fails.

Proposed solution

Type checking on handlers at startup
To prevent users from wrongly using IEndpointInstance and IMessageSession on their handlers we should check on startup and throw an exception if any handler has a ctor/property dependency in said types. The exception should tell them to use the handler context instead.

[johnsimons]

Isn't this just a case of stopping adding IEndpointInstance to the container ?

[danielmarbach]

@johnsimons I setup a meeting for monday. Will add the necessary context after that meeting to this issue.

[danielmarbach]

Tagged the platform development issue as Consider for Promotion

[indualagarsamy]

Comments from @ppittle

@andreasohlund, throwing a run-time exception might not be the optimal solution in this case. Checking at run-time will incur a (albeit small) performance penalty. But more importantly it defers providing uses with feed-back until run-time.
For cases like this, it would be nice to give a compilation error. With Roslyn Code Analyzers you should be able to write a rule that captures this scenario and provides users faster feedback.
Might not work in all scenarios (ie old versions of VS / .net framework), but thought I'd throw it out there.

[indualagarsamy]

Comments from @timbussmann

@ppittle we also discussed Roslyn Analyzers and we think they would be a great tool to avoid Runtime exceptions. It's all about effort/value and this issue aims at "finding most mistakes with the least possible effort" while a Roslyn Anaylzer will mostly add more "comfort" by providing compile time errors. A Roslyn Analyzer should be able to find more "invalid" usages of IMessageSession but it also can't provide a 100% detection rate. The good thing is that we can always provide a Roslyn Analyzer at a later stage while still providing some built-in mechanisms to avoid most-common mistakes.

[indualagarsamy]

Comments from @andreasohlund

Checking at run-time will incur a (albeit small) performance penalty

Agreed, but this will only happen once at startup so I think this is something that would be insignificant since we already to a fair amount of reflection already.

[andreasohlund]

@Particular/nservicebus-maintainers I propose that we do this as part of maintainer budgets, ok?

[danielmarbach]

Let's get this done next week.

[andreasohlund]

PR has been merged. @indualagarsamy there was some debate regarding the exception message.

https://github.com/Particular/NServiceBus/blob/release-6.0.0/src/NServiceBus.Core/Unicast/MessageHandlerRegistry.cs#L163

Please review and close if you're happy with the final incarnation.

[andreasohlund]

@indualagarsamy closing this, let me know if you want the obsolete message to be changed

[indualagarsamy]

@andreasohlund - no changes needed. Looks good, thanks.

[ppittle]

Hi all! Wanted to chime in on this once more:

If you're planning on throwing an exception if an IEndpointInstance is injected into a Handler, I need either an opt-out mechanism. or to verify wrappers won't throw exceptions.

I have an edge case (and please let me know if I'm not implementing this correctly) where I need to shard my transports to alleviate a message throughput bottleneck. I'm using Multi Hosting and need to have a handler that can proxy messages between Transports.

I have created wrapper interfaces to distinguish between endpoints and then am injecting the wrappers into my handlers.

So I have some code like this (very slimmed down example for illustration):

 public interface ILanEndpointInstance{ IEndpointInstance Endpoint {get;} }
 public interface IWanEndpointInstance { IEndpointInstance Endpoint {get;} }

 // Handles messages sent to the "Wan" Endpoint
 public class WanHandler : IHandleMessage<MessageFromWan>
 {
       // Constructor Inject the Lan endpoint instance
       private readonly ILanEndpointInstance _lanEndpoint;

       public async Task Handle(MessageFromWan message, IMessageHandlerContext context)
       {
            //proxy message to Lan transport
            await _lanEndpointInstance.Endpoint.SendLocal(new MessageToLan
            {
                WanOriginatorAddress = context.ReplyToAddress
            });             
       }     
  }

 //Handles messages sent to the "Lan" Endpoint
 public class LanHandler : IHandleMessage<MessageToLan>
 {
     // Constructor Inject the Wan endpoint instance
     private readonly IWanEndpointInstance _wanEndpoint;

     public async Task Handle(MessageFromWan message, IMessageHandlerContext context)
     { 
          //send message back to Wan originator 
          await _wanEndpoint.Endpoint.Send(
               message.WanOriginatorAddress,
              new Result());
      }     
}

A little unorthodox, but in this case I believe its necessary to inject the Endpoint Instances. The expectation is I will be generating lots of traffic on the ILanEndpointInstance. I want to spin up a dedicated Transport for that traffic so it doesn't overwhelm the IWanEndpointInstance Transport, which will be shared my many more Hosts.

[timbussmann]

Afaik, the current implementation only checks for MessageSession properties & direct ctor dependencies. The wrappers as you defined them wouldn't raise an exception @ppittle.