[Service Bus] Rework MessageLockCancellationToken/SessionLockCancellationToken to use events

[JoshLove-msft]

After reviewing the proposed API with the architects, we received feedback that it would be more idiomatic to have a pattern like the following:

async Task ProcessMessage(ProcessMessageEventArgs args)
{
    using var cts = CancellationTokenSource.CreateLinkedTokenSource(args.CancellationToken);
    args.MessageLockLostAsync += cts.Cancel();

    await FooAsync(args.Message, cts.Token);
}

This means we would need to replace the MessageLockCancellationToken/SessionLockCancellationToken properties with events that can be subscribed to.

/cc @danielmarbach

[danielmarbach]

In what sense is this more idiomatic? Can you add more context? Is it because the args provide more than one token that makes it harder to for example use token forwarding analysers?

The thing with the token approach is that you can easily combine them into linked sources or decide which one to forward depending on the use case. Since cooperative cancellation is a well known pattern that allows forwarding (or leaving out the token where appropriate) this seems to me more aligned with the use cases we aimed for. The token approach also allows to conveniently use token registrations with the using statement that then get auto disposed when the handler exits.

Subscribing to an event within an event arg is at least to me a more exotic example that I personally haven't seen used often (might be a lack of knowledge on my end though). It might also lead to more confusion by users because they might try to unregister the handler but wouldn't find a convenient place to do so.

[JoshLove-msft]

In what sense is this more idiomatic? Can you add more context? Is it because the args provide more than one token that makes it harder to for example use token forwarding analysers?

The feedback was that the existing cancellation token makes sense as it signalled when the processor is shutting down. The new token doesn't really represent an operation being cancelled but rather it is used to signal that some event has occurred (the lock was lost), and therefore an event-based API would be more appropriate. I agree with your points about linking tokens and it being fairly convenient with the two token approach, but I think users can still create a linked token pretty easily with the callback approach. Also agree with your point about users thinking they need to unregister, but I think we can clarify in docs that the event goes out of scope outside of the ProcessMessage callback.

[danielmarbach]

The new token doesn't really represent an operation being cancelled but rather it is used to signal that some event has occurred (the lock was lost), and therefore an event-based API would be more appropriate.

Hmmm... My mental model was that from a receive operation perspective the receive operation is signaled as "canceled". Under peek lock, once the lock is lost, continuing the receive operation doesn't make much sense since any attempt to complete would fail anyway with a lock lost which effectively means "an operation has been canceled". That's why I can't follow the reasoning of

and therefore an event-based API would be more appropriate

When you look at the angle of shutdown you could apply the same argument too. Essentially the token represents the shutdown event and therefore it should also be "expressed as an event instead".

Or in other words:

Shutdown Requested --> Receive Operation should be cancelled expressed by toggling the cancellation token
LockLost --> Receive Operation should be cancelled expressed by toggling the other cancellation token

So in essence both the shutdown and the lock lost "trigger" fall into the area of cooperative cancellation from the perspective of the receive operation for me. And if you are interested in both events you create a combined token.

FYI I'm not hung up on the current solution. I'm trying to poke holes into the arguments presented here ;)

[JoshLove-msft]

When you look at the angle of shutdown you could apply the same argument too. Essentially the token represents the shutdown event and therefore it should also be "expressed as an event instead".

You have a point here. I don't think it is clear cut in terms of one approach vs the other.

Or in other words:
Shutdown Requested --> Receive Operation should be cancelled expressed by toggling the cancellation token
LockLost --> Receive Operation should be cancelled expressed by toggling the other cancellation token
So in essence both the shutdown and the lock lost "trigger" fall into the area of cooperative cancellation from the perspective of the receive operation for me. And if you are interested in both events you create a combined token.
FYI I'm not hung up on the current solution. I'm trying to poke holes into the arguments presented here ;)

Totally fair. I agree that cooperative cancellation is the primary scenario we want to support here, but it may not be the only scenario. Expressing the lock lost as an event opens up the door to do other actions when the lock is lost without the user needing to do ct.Register(...), while still providing a straightforward way to handle the linked token:

async Task ProcessMessage(ProcessMessageEventArgs args)
{
    using var cts = CancellationTokenSource.CreateLinkedTokenSource(args.CancellationToken);
    args.MessageLockLostAsync += cts.Cancel();

    await FooAsync(args.Message, cts.Token);
}

In the end, I think it is a pretty close call between the two approaches - they both allow for the same functionality. But the architects felt that using events here is a bit cleaner. I think your point about the multiple cancellation tokens is valid as well - that makes it feel a bit more difficult to reason about for users.

I can throw up a PR to make the API change (unless you would prefer to do this) - you already did all of the heavy lifting - I can just trigger the event when the token is signaled by using a registration.

[JoshLove-msft]

Another benefit of the event-based approach is that we can subclass EventArgs to allow us to flow through context data to the event. It is probably interesting to include the exception, and possibly the LockedUntilTime.

[danielmarbach]

I can throw up a PR to make the API change (unless you would prefer to do this) - you already did all of the heavy lifting - I can just trigger the event when the token is signaled by using a registration.

Thanks. I'm currently on vacation and taking a bit of a break from coding. I'm back in two weeks and could pick it up if it hasn't been done until then but I guess you want to fix this asap