search

Azure / azure-amqp

AMQP C# library
Other
94 stars 70 forks source link

Revert new abstract method (breaking change) #215

Closed jsquire closed 2 years ago

jsquire commented 2 years ago

Summary

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.

jsquire commented 2 years ago

@xinchen10: The previous set of changes overlooked that a new abstract method was added to Singleton, which breaks callers bound to earlier versions because they do not have an implementation for it. To get around this, I had to invert the delegation flow.

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.

jsquire commented 2 years ago

//cc: @JoshLove-msft, @AlexGhiondea

xinchen10 commented 2 years ago

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.

JoshLove-msft commented 2 years ago

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