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

AbandonAsync doesn't check that token has been already released #647

Closed n1l closed 5 years ago

n1l commented 5 years ago

Hi, first of all I wish to know if I use ServiceBus client in incorrect way.

I use a service fabric cluster and I wish to restart a process of a service after an exception. So I wish to throw an exception to the top level of a stack trace. And I wish to abandon my invalid message to a dead letter queue, so I call abandon explicitly. Also I marked AutoComplete - false, I wish to control the way of handling my messages. But after I called abandon in code and re-throw an exception I got a MessageLockLostException, because abandon also will be called if a user handler throws an exception. And it won't check that a lock has been released.

This repo will provide my idea in detail: https://github.com/n1l/abandon-async-example

Actual Behavior

  1. Catch exception
  2. call queueClient.AbandonAsync
  3. re-throw exception

Result: - Current exception from user and MessageLockLostException has been thrown

Expected Behavior

  1. Catch exception
  2. call queueClient.AbandonAsync
  3. re-throw exception

Result: - Only current exception from user that has been thrown

Versions

n1l commented 5 years ago

@SeanFeldman what do you think?

SeanFeldman commented 5 years ago

@n1l thanks for the question. I believe this is done by design. Message Handler API is a message pump that is supposed to work non-stop. When a user callback is throwing an exception, it's communicated to the exception callback that is intended for logging and registering information that would be needed later for investigation. If you need a custom control over the pump, you're might be better off to create your won message pump with MessageReceiver.

@nemakam would you agree with the above?

n1l commented 5 years ago

Hi, @SeanFeldman, thank you for replay. I've double checked my code, and I've found that I receive a MessageLockLost even if my handler does not re-throw an exception but I throw an exception after abandon.

For example if I change my handler to: static Task ExceptionReceivedHandler(ExceptionReceivedEventArgs exceptionReceivedEventArgs) { Console.WriteLine(exceptionReceivedEventArgs.Exception); return Task.CompletedTask; } I'll continue get such exception.

SeanFeldman commented 5 years ago

The processing callback is:

_queue.RegisterMessageHandler(async (message, token) =>
{
  try
  {
    throw new Exception("Test");
  }
  catch (Exception)
  {
    await _queue.AbandonAsync(message.SystemProperties.LockToken);
    throw;
  }
}

No matter what you do in the callback, including throwing/re-throwing, the callback is wrapped in try/catch by the ASB client and will never throw. That's the idea behind message pump - it should run no matter what. If that's what you're trying to achieve, I'm afraid the built-in pump will not help you. Unless it's something different and I don't quite understand the scenario.

n1l commented 5 years ago

Thank alot, @SeanFeldman!

SeanFeldman commented 5 years ago

No worries. If you think this is answered, then perhaps close the issue. Cheers.

n1l commented 5 years ago

Yeah, @SeanFeldman , I think I understood the idea. It can be closed.