Azure / azure-service-bus-dotnet

☁️ .NET Standard client library for Azure Service Bus
https://azure.microsoft.com/services/service-bus
Other
235 stars 120 forks source link

NullReferenceException when accessing message.Size property #648

Closed dolly22 closed 5 years ago

dolly22 commented 5 years ago

Actual Behavior

When accessing empty message (message.Body is null) size property NullReferenceException is thrown.

Expected Behavior

Getting NullReferenceException was pretty unexpected. I would expect to get either 0 size or size of message properties

Versions

nuget 3.3.0 / netstandard2

SeanFeldman commented 5 years ago

Technically, the body can be either null or an empty array. Returning zero for both use cases would be wrong.

@dolly22 if your code needs to handle null body messages, shouldn't it then even bother with the Size property? Could you please share the code where you have a null body and need to use Size? I'd like to understand better what's happening from a usage perspective. Thanks.

SeanFeldman commented 5 years ago

@dolly22 ping

dolly22 commented 5 years ago

I'm accessing Size property in MessageHandler callback for the purpose of some diagnostic logging (create logger scope with MessageId context, log basic message properties - id, retry, created, size, contenttype, ...)

So i have some pretty innocent looking code like this...

using (logger.BeginScope(new Dictionary<string, string>()
{
    ["MessageId"] = message.MessageId,
}))
{
    logger.LogDebug($"[{context.EntityPath}, id: {message.MessageId}, contentType: '{message.ContentType}'] start processing, try #{message.SystemProperties.DeliveryCount}, label '{message.Label}', size {message.Size} bytes, enqueued {message.SystemProperties.EnqueuedTimeUtc}");
    ...
}

that throws NullReferenceException when message body is null.

I also use some abstraction on top of message sender/handler for large messages. For such message service bus message body is null and content is stored as azure blob (only blob SAS uri is stored in message UserProperties).

SeanFeldman commented 5 years ago

I also use some abstraction on top of message sender/handler for large messages. For such message service bus message body is null and content is stored as azure blob (only blob SAS uri is stored in message UserProperties).

When it comes to abstraction like the one you've mentioned, it should play by the rules of the ASB client. Which is expecting a message sent out to be delivered and received with the contents. Sounds like the abstraction you're using is not doing that, causing message body to be null and therefore breaking the assumption client is making.

Are you using ServiceBus.AttachmentPlugin by any chance? If you are, it's a plugin that should be registered on both, sending and receiving ends, ensuring the content of the message is rehydrated.

That's why I think there's something off about this request. The entire message is either a valid message, or nothing (null is a possible value when receiving a message using MessageReceiver.ReceiveAsync().

dolly22 commented 5 years ago

I'm not using AttachmentPlugin but some inhouse abstraction that uses streams to handle large content so message.Body byte[] is completely ignored when sending/receiving such messages.

I was supprised by this behaviour when migrating code from Microsoft.ServiceBus.Messaging / BrokeredMessage where this was completely ok. I can easily adapt/workaround to this. I'm just reporting this was pretty unexpected from someone who used this library.

For me it's supprising that this code throws NullReferenceException...

var msg = new Message();
Console.WriteLine(msg.Size);

yet it's completely valid to create/send/receive such message, just invalid to access it's Size. If this is expected than I (as library consumer) would prefer Size property not to exist at all and be forced to use msg.Body.Length directly.

SeanFeldman commented 5 years ago

If this is expected than I (as library consumer) would prefer Size property not to exist at all and be forced to use msg.Body.Length directly.

In case msg.Body is null, you'd still need to check rather than access .Length directly.

I'm looking at it from a different perspective. The old library returned zero when no body was provided. In that way, this is a regression from the expected behaviour when upgrading from the old client. Based on that, this would be categorized as a bug.