Skip to content

Commit

Permalink
Revert new abstract method (breaking change) (#215)
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
jsquire authored Jan 8, 2022
1 parent ec02c6e commit b2e1d20
Show file tree
Hide file tree
Showing 3 changed files with 27 additions and 3 deletions.
7 changes: 7 additions & 0 deletions Microsoft.Azure.Amqp/Amqp/Cbs/AmqpCbsLink.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.Azure.Amqp
{
using System;
using System.Collections.Generic;
using System.ComponentModel;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Azure.Amqp.Framing;
Expand Down Expand Up @@ -121,6 +122,12 @@ protected override Task<RequestResponseAmqpLink> OnCreateAsync(TimeSpan timeout,
this);
}

// Deprecated, but needs to stay available to avoid
// breaking changes. The attribute removes it from
// any code completion listings and doc generation.
[EditorBrowsable(EditorBrowsableState.Never)]
protected override Task<RequestResponseAmqpLink> OnCreateAsync(TimeSpan timeout) => OnCreateAsync(timeout, CancellationToken.None);

protected override void OnSafeClose(RequestResponseAmqpLink value)
{
value.Abort();
Expand Down
7 changes: 7 additions & 0 deletions Microsoft.Azure.Amqp/Amqp/FaultTolerantAmqpObject.cs
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
namespace Microsoft.Azure.Amqp
{
using System;
using System.ComponentModel;
using System.Threading;
using System.Threading.Tasks;

Expand Down Expand Up @@ -54,6 +55,12 @@ protected override async Task<T> OnCreateAsync(TimeSpan timeout, CancellationTok
return amqpObject;
}

// Deprecated, but needs to stay available to avoid
// breaking changes. The attribute removes it from
// any code completion listings and doc generation.
[EditorBrowsable(EditorBrowsableState.Never)]
protected override Task<T> OnCreateAsync(TimeSpan timeout) => OnCreateAsync(timeout, CancellationToken.None);

protected override void OnSafeClose(T value)
{
this.closeObject(value);
Expand Down
16 changes: 13 additions & 3 deletions Microsoft.Azure.Amqp/Singleton.cs
Original file line number Diff line number Diff line change
Expand Up @@ -136,10 +136,20 @@ protected virtual bool IsValid(TValue value)
// Deprecated, but needs to stay available to avoid
// breaking changes. The attribute removes it from
// any code completion listings and doc generation.
//
// This method used to be abstract, but can no longer be to
// to avoid callers bound to the new versions from needing to
// implement, despite it being deprecated.
[EditorBrowsable(EditorBrowsableState.Never)]
protected virtual Task<TValue> OnCreateAsync(TimeSpan timeout) => OnCreateAsync(timeout, CancellationToken.None);

protected abstract Task<TValue> OnCreateAsync(TimeSpan timeout, CancellationToken cancellationToken);
protected virtual Task<TValue> OnCreateAsync(TimeSpan timeout) => throw new NotImplementedException();

// This approach is not ideal, as the cancellation token is silently
// ignored if this method is not overridden in derived classes.
//
// However, it is necessary to avoid breaking changes for callers bound
// to earlier versions, as they will not have implemented this new method,
// so it cannot be abstract and must delegate to the legacy implementation.
protected virtual Task<TValue> OnCreateAsync(TimeSpan timeout, CancellationToken cancellationToken) => OnCreateAsync(timeout);

protected abstract void OnSafeClose(TValue value);

Expand Down

0 comments on commit b2e1d20

Please sign in to comment.