Add the possibility to modify properties when abandoning a message

[0xced]

This is useful to store exception details when abandoning a message and still keeping the convenience of registering a message or session handler with AutoComplete = true.

Replaces #646

[SeanFeldman]

I like this approach better. It's not as invasive as #646 and does not complicate APIs.

[SeanFeldman]

Make it latest

[0xced]

I set it explicitly to 7.3 because C# 7.3 is needed for the tests to compile after I added the second MessageHandlerOptions constructor. See Unable to invoke overload when argument is func of non generic task and supplied as method which describes the exact same situation where the compiler generates errors if LangVersion is smaller than 7.3:

OnSessionQueueTests.cs(63, 47): [CS0407] 'Task OnSessionQueueTests.ExceptionReceivedHandler(ExceptionReceivedEventArgs)' has the wrong return type
OnSessionQueueTests.cs(147, 47): [CS0407] 'Task OnSessionQueueTests.ExceptionReceivedHandler(ExceptionReceivedEventArgs)' has the wrong return type
OnSessionTopicSubscriptionTests.cs(104, 47): [CS0407] 'Task OnSessionTopicSubscriptionTests.ExceptionReceivedHandler(ExceptionReceivedEventArgs)' has the wrong return type
SenderReceiverClientTestBase.cs(262, 43): [CS0407] 'Task SenderReceiverClientTestBase.ExceptionReceivedHandler(ExceptionReceivedEventArgs)' has the wrong return type
SenderReceiverClientTestBase.cs(292, 43): [CS0407] 'Task SenderReceiverClientTestBase.ExceptionReceivedHandler(ExceptionReceivedEventArgs)' has the wrong return type

But I’m happy to bump it to latest if you think it’s better.

[SeanFeldman]

Test project is free to use the latest and greatest. Rather than bumping it manually all the time, it could be on the latest all the time.

[nemakam]

@0xced - Lets discuss this before we take this further. I don't want a lot of hard work to go wasted. What is the intention, what is that we are trying to solve, and is it going to complicate things? Are we giving promises that everytime we run into exception, we would be updating the properties? There might be lot of exceptions that are raised for which we will not be able to update them. For example, CommunicationException, or say LockLostException etc. What if autocomplete fails and that is the reason we raise an exception? There are lot of places where we call the exception handler. What is the expectation from the handler code in those cases? How will it distinguish between exception thrown from the message handler, vs exception thrown from doing auto complete?

[0xced]

Are we giving promises that everytime we run into exception, we would be updating the properties?

Only when the exception happens in the user callback, i.e. the handler passed to void IReceiverClient.RegisterMessageHandler(Func<Message, CancellationToken, Task> handler, …)

There might be lot of exceptions that are raised for which we will not be able to update them. For example, CommunicationException, or say LockLostException etc.

I’m not sure about ServiceBusCommunicationException, but the message is not abandoned if the exception is a MessageLockLostException:

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

Lines 153 to 157 in 4e6ca73

	// Nothing much to do if UserCallback throws, Abandon message and Release semaphore.
	if (!(exception is MessageLockLostException))
	{
	await this.AbandonMessageIfNeededAsync(message, propertiesToModify).ConfigureAwait(false);
	}

By the way, I wonder how a MessageLockLostException could be thrown from the user callback. 🤔

There are lot of places where we call the exception handler. What is the expectation from the handler code in those cases?

I have documented this: the returned dictionary is ignored. Maybe this documentation needs improvement?

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

Lines 43 to 44 in 4e6ca73

	/// When the exception happens during user callback, the returned dictionary is passed to <see cref="IReceiverClient.AbandonAsync"/>.
	/// For other actions, the returned dictionary is ignored.

How will it distinguish between exception thrown from the message handler, vs exception thrown from doing auto complete?

I’m not sure I fully understand your concern. I think it doesn’t matter since the returned dictionary is ignored when RaiseExceptionReceived is called when auto completion fails:

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

Lines 210 to 224 in 4e6ca73

	async Task CompleteMessageIfNeededAsync(Message message)
	{
	try
	{
	if (this.messageReceiver.ReceiveMode == ReceiveMode.PeekLock &&
	this.registerHandlerOptions.AutoComplete)
	{
	await this.messageReceiver.CompleteAsync(new[] { message.SystemProperties.LockToken }).ConfigureAwait(false);
	}
	}
	catch (Exception exception)
	{
	await this.RaiseExceptionReceived(exception, ExceptionReceivedEventArgsAction.Complete).ConfigureAwait(false);
	}
	}

If you meant from the user of the library point of view, the ExceptionReceivedEventArgs passed to the exception handler has a ExceptionReceivedContext.Action which describes the action, i.e. Complete, Abandon, UserCallback, Receive, RenewLock, AcceptMessageSession or CloseMessageSession.

[0xced]

@nemakam Any thoughts on this?