ISenderClient:SendAsync(IList<Message> messageList) throws null reference when messageList is empty

[lukasz-pyrzyk]

Actual Behavior

1. SendAsync throws null reference exception when given collection of messages is empty.

Expected Behavior

1. No exception should be thrown.

Simplest repro

        [Fact]
        [DisplayTestMethodName]
        void Empty_Collection_Of_Messages_Returns_Empty_AMQP_Message()
        {
            var messages = new Message[0];

            var batchedMessage = AmqpMessageConverter.BatchSBMessagesAsAmqpMessage(messages);

            Assert.NotNull(batchedMessage);
        }

System.NullReferenceException : Object reference not set to an instance of an object.
at Microsoft.Azure.ServiceBus.Amqp.AmqpMessageConverter.BatchSBMessagesAsAmqpMessage(IEnumerable`1 sbMessages)
at Microsoft.Azure.ServiceBus.UnitTests.AmqpConverterTests.Empty_Collection_Of_Messages_Returns_Empty_AMQP_Message()

Reason

azure-service-bus-dotnet/src/Microsoft.Azure.ServiceBus/Amqp/AmqpMessageConverter.cs

Line 59 in 6f144e9

if (dataList == null)

dataList is initalized in a foreach loop, so when there is no messages it will be null.
Later one, its used by amqpMessage = AmqpMessage.Create(dataList); what causes null reference exception.

Versions

• OS platform and version: Windows 10
• .NET Version: .NET Core 2.1
• NuGet package version or commit ID: 3.1.0-preview, master

I can pick up this issue if you want.

[axisc]

Sorry for the late response. If you'd still like to fix it please proceed and submit a pull request.

thank you !

[thomaslevesque]

I just hit the same bug

[lukasz-pyrzyk]

Oh, now i have the reason to fix it 😅 I will do it this week

[thomaslevesque]

Later one, its used by amqpMessage = AmqpMessage.Create(dataList); what causes null reference exception.

Are you sure about this? Looking at the code for AmqpMessage, I don't see how it could throw, since it doesn't dereference dataList.

[thomaslevesque]

Anyway, in the meantime the workaround is simple. Don't call SendAsync if you don't have any message...

[lukasz-pyrzyk]

I think in the past it was throwing on the amqpMessage = AmqpMessage.Create(dataList); when dataList was null. Currently it throws later, on the firstMessage.MessageId. Not sure if I was mistaken or Microsoft.Azure.Amqp.AmqpMessage was updated and AmqpMessage.Create changed

[SeanFeldman]

SendAsync() should not allow sending empty or null collection.
Guard for null collection is in place. Guard for empty collection is missing.
PR #641 will address this.

[thomaslevesque]

SendAsync() should not allow sending empty or null collection.

I would have expected SendAsync() with an empty collection to be a no-op

[SeanFeldman]

I would have expected SendAsync() with an empty collection to be a no-op

Interesting. Could you elaborate why an empty collection would be a no-op, but a null message would throw? I'm trying to understand the rationale behind this though. Both imo are invalid cases and messages should not be attempted to be dispatched. But I might not be seeing another pov. Thanks.

[thomaslevesque]

Interesting. Could you elaborate why an empty collection would be a no-op, but a null message would throw? I'm trying to understand the rationale behind this though. . But I might not be seeing another pov. Thanks.

Well, the method expects a list. Null isn't a list, so obviously it should be rejected, but an empty list is still a valid list. There are many examples of methods in the BCL that accept an empty collection: List<T>.AddRange, collection constructors, Linq methods, Task.WaitAll, String.Concat, Parallel.ForEach, Stream.Write...

[SeanFeldman]

Convinced. PR #642 will replace the original PR.

[thomaslevesque]

@SeanFeldman thanks!