Revert new abstract method (breaking change) #215
Conversation
The focus of these changes is to revert the breaking change in Singleton where a new abstract OnCreateAsync overload was added. Doing so required trade-offs which were not ideal. To ensure that callers build against older versions of the AMQP library could continue to function, the new overload `"OnCreateAsync(TimeSpan, CancellationToken)` is now forwarding to the legacy overload `OnCreateAsync(TimeSpan)` and will silently ignore the cancellation token. Because we cannot be sure which `OnCreateAsync` overload derived classes are calling, both have been marked `virtual` rather than any `abstract`; this required having the legacy overload throw when not implemented, since that is the target of delegation. The defined derived classes `FaultTolerantAmqpObject` and `AmqpCbsLink` have been adjusted to implement both overrides, with the legacy overload calling into the new overload with `CancellationToken.None` specified. This is intended to ensure that callers bound to older versions continue to work.
|
@xinchen10: The previous set of changes overlooked that a new abstract method was added to I'd appreciate your thoughts on the approach and whether you see any further gaps from the cancellation token support changes that I may have overlooked. In the meantime, we're going to do some further validation on our end since the initial scenario did not cover this case. |
|
//cc: @JoshLove-msft, @AlexGhiondea |
|
Changes look good to me. I am curious how you use Singleton class. Though it is public, it is not meant to be used outside of this library, as indicated by the xml comments. If you use FaultTolerantAmqpObject or AmqpCbsLink, it should work as no breaking changes were introduced on them. |
It is used here - https://github.com/Azure/azure-sdk-for-net/blob/main/sdk/servicebus/Azure.Messaging.ServiceBus/src/Amqp/AmqpTransactionEnlistment.cs#L14 |
Summary
The focus of these changes is to revert the breaking change in Singleton where a new abstract
OnCreateAsyncoverload was added. Doing so required trade-offs which were not ideal.To ensure that callers build against older versions of the AMQP library could
continue to function, the new overload
OnCreateAsync(TimeSpan, CancellationToken)is now forwarding to the legacy overloadOnCreateAsync(TimeSpan)and will silently ignore the cancellation token.Because we cannot be sure which
OnCreateAsyncoverload derived classes are calling, both have been markedvirtualrather than anyabstract; this required having the legacy overload throw when not implemented, since that is the target of delegation.The defined derived classes
FaultTolerantAmqpObjectandAmqpCbsLinkhave been adjusted to implement both overrides, with the legacy overload calling into the new overload withCancellationToken.Nonespecified. This is intended to ensure that callers bound to older versions continue to work.