Re-introduce default cancellationToken for PurgeInstanceAsync for backwards compatibility
#276
Conversation
…ackwards compatibility
| @@ -337,7 +337,7 @@ public virtual Task ResumeInstanceAsync(string instanceId, CancellationToken can | |||
| public abstract AsyncPageable<OrchestrationMetadata> GetAllInstancesAsync(OrchestrationQuery? filter = null); | |||
|
|
|||
| /// <inheritdoc cref="PurgeInstanceAsync(string, PurgeInstanceOptions, CancellationToken)"/> | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation) | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation = default) | |||
There was a problem hiding this comment.
From a binary compatibility perspective, this is still a breaking change. We need to introduce a virtual method that doesn't include the CancellationToken parameter at all.
public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation)
=> this.PurgeInstanceAsync(instanceId, CancellationToken.None);There was a problem hiding this comment.
Wait, maybe my suggestion isn't the right one. I'd need to double check where the failure is coming from.
There was a problem hiding this comment.
I'll be honest, I haven't had the chance to validate my diff yet either. Just FYI
|
Before I forget - let's validate that there weren't other similar breaking changes (in other client methods) that the test didn't catch but still need resolving. |
| @@ -337,7 +337,7 @@ public virtual Task ResumeInstanceAsync(string instanceId, CancellationToken can | |||
| public abstract AsyncPageable<OrchestrationMetadata> GetAllInstancesAsync(OrchestrationQuery? filter = null); | |||
|
|
|||
| /// <inheritdoc cref="PurgeInstanceAsync(string, PurgeInstanceOptions, CancellationToken)"/> | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation) | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation = default) | |||
There was a problem hiding this comment.
Adding the default param back will cause ambiguous overload resolution with
public virtual Task<PurgeResult> PurgeInstanceAsync(
string instanceId, PurgeInstanceOptions? options = null, CancellationToken cancellation = default)when calling .PurgeInstanceAsync(string) (which of the 2 default param methods is it to use?)
We will also need to remove the default param from options in that other method.
| @@ -337,7 +337,7 @@ public virtual Task ResumeInstanceAsync(string instanceId, CancellationToken can | |||
| public abstract AsyncPageable<OrchestrationMetadata> GetAllInstancesAsync(OrchestrationQuery? filter = null); | |||
|
|
|||
| /// <inheritdoc cref="PurgeInstanceAsync(string, PurgeInstanceOptions, CancellationToken)"/> | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation) | |||
| public virtual Task<PurgeResult> PurgeInstancesAsync(string instanceId, CancellationToken cancellation = default) | |||
There was a problem hiding this comment.
Oh! the error message you see of no overload resolution is because of the typo in #267
|
Closing this as the expected bug fix was merged. |
Follow-up to: #262
It would appear that in the refactoring above we accidentally made a breaking change to our
PurgeInstancesAsyncAPI, dropping the declaration of aPurgeInstanceAsync(string instanceId, CancellationToken cancellation = default)method, which is overriden here.At a glance, I suspect this is the reason for our smoke test failures in the
worker.extensions.durabletaskpackage: https://github.com/Azure/azure-functions-durable-extension/actions/runs/8428205010/job/23080260061?pr=2772Failure trace:
This PR is a naive attempt at trying to fix them.