Throw a meaningful exception when a binding name is too long

[mauroservienti]

A customer came to me with the following error at endpoint startup:

RabbitMQ.Client.Exceptions.WireFormattingException: Short string too long; UTF-8 encoded length=260, max=255
   at RabbitMQ.Client.Impl.WireFormatting.WriteShortstr(NetworkBinaryWriter writer, String val)
   at RabbitMQ.Client.Framing.Impl.QueueBind.WriteArgumentsTo(MethodArgumentWriter writer)
   at RabbitMQ.Client.Impl.Command.TransmitAsSingleFrame(Int32 channelNumber, Connection connection)

The customer was using DirectRoutingTopology and one binding was too long. The thing is that all their events inherit from a base class; thus the binding name was namespace-base-class.namespace-derived-class. They added a new event where the entire string was 5 chars longer than allowed. Being the above error useless, it took us, the customer and myself, nearly two hours to figure out what was going on and identify the source of the error.

Given that we are creating those bindings, we should wrap the broker exception in a custom one adding some details, such as the entity name that is causing the failure.

[adamralph]

👍 thanks for raising @mauroservienti. I've renamed to reflect this as a new "feature".

[mauroservienti]

Thank you

[andreasohlund]

The description mentions DirectRoutingTopology which is pretty much no longer used but I assume the same thing could happen with the conventional one?

[bording]

The description mentions DirectRoutingTopology which is pretty much no longer used

There's nothing that we've done that actually prevents people from using it, so I'm not sure that's actually true.

I assume the same thing could happen with the conventional one?

Yeah, there are some length limitations in there as well, so this could apply more generally. I've started looking at what could be done here.

[bording]

Looking at this a bit more, I do think there is more we can do here, but if we change the exception type we expose to users by wrapping the client exceptions in something like Exception instead, that is technically a breaking behavior change.

Would be be okay with going from throwing RabbitMQ.Client.Exceptions.WireFormattingException or RabbitMQ.Client.Exceptions.OperationInterruptedException to just Exception in a minor release?

[andreasohlund]

I personally think a minor is fine since there is no way for us to guarantee that we don’t change code in a way that other exceptions couldn’t be thrown. But I think that you already answered your self Particular/PlatformDevelopment#2675 (comment) :)

…

On Thu, 21 Mar 2019 at 17:15, Brandon Ording ***@***.***> wrote: Looking at this a bit more, I do think there is more we can do here, but if we change the exception type we expose to users by wrapping the client exceptions in something like Exception instead, that is technically a breaking behavior change. Would be be okay with going from throwing RabbitMQ.Client.Exceptions.WireFormattingException or RabbitMQ.Client.Exceptions.OperationInterruptedException to just Exception in a minor release? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#453 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHoZBiy6ASyt1UhFUjdJzEqEVy25PP2ks5vY7ARgaJpZM4QlcvF> .

[bording]

I do generally think that changing the exception type the way we'd need to here is a break. Someone could have code that is catching the existing exceptions and doing something in the catch blocks.

If we changed to Exception, then those catch blocks would no longer catch the exceptions, and we'd have broken them.

That's why I'm thinking we'd really need to wait for a new major to do this.

[andreasohlund]

I’m happy with that decision

…

On Thu, 21 Mar 2019 at 17:35, Brandon Ording ***@***.***> wrote: I do generally think that changing the exception type the way we'd need to here is a break. Someone could have code that is catching the existing exceptions and doing something in the catch blocks. If we changed to Exception, then those catch blocks would no longer catch the exceptions, and we'd have broken them. That's why I'm thinking we'd really need to wait for a new major to do this. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#453 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHoZFNASo2f5Yw8FiEagI-LijcXGztaks5vY7TsgaJpZM4QlcvF> .

[bording]

FYI to make this work the way we'd want to, we'd also need to stop swallowing exceptions in https://github.com/Particular/NServiceBus.RabbitMQ/blob/develop/src/NServiceBus.Transport.RabbitMQ/Routing/ConventionalRoutingTopology.cs#L138-L140
and https://github.com/Particular/NServiceBus.RabbitMQ/blob/develop/src/NServiceBus.Transport.RabbitMQ/Routing/DirectRoutingTopology.cs#L72-L76

which I think would probably be a good thing anyway, but this really gets outside the realm of what should be done in a minor.

I'll remove this from the 5.1.0 milestone, and we can revisit when we release a new major.

[tmasternak]

This will be solved when this PR is merged into the Rabbit Client library.

[bording]

The client PR gets us most of the way there since it means the exception thrown from the client will contain the offending value, so it should be more obvious which string it is that is causing the problem.

We could leave this open to further improve things, trying to surface a "queue name too long" or "binding too long" exception that points user even more in the right direction, but I'm not sure if that's worth doing or not.