Validate that timeout is positive in WaitForOrchestrationAsync, and enable nullable checks #910
Conversation
…nstanceCompletion
| @@ -1857,6 +1856,13 @@ async Task<PurgeResult> IOrchestrationServicePurgeClient.PurgeInstanceStateAsync | |||
| throw new ArgumentException(nameof(instanceId)); | |||
| } | |||
|
|
|||
| if (timeout < TimeSpan.Zero) | |||
There was a problem hiding this comment.
Should we accept negative values? There seems to be a precedence that negative implies infinite for timeouts. Especially since Timeout.InfiniteTimeSpan is negative.
There was a problem hiding this comment.
Maybe, I think it's up for debate, and I no strong feelings on this. I simply noticed that the code today assumes the timeout will be positive, and so I made that assumption explicit by validating the input.
Using a combination of Datetime.now and the provided timespan, we can modify the currrent implementation to work with infinite timespans as well.
Would you prefer if we accepted negative timespans? I see no issue with that
There was a problem hiding this comment.
I think it would be a better experience to accept negative as an infinite timeout.
There was a problem hiding this comment.
I'd say let's be specific and allow for Timeout.InfiniteTimeSpan to mean infinite, whereas we throw for any other negative value. I believe this is mostly consistent with other .NET APIs and helps us differentiate an explicit desire for infinite vs. an accidental negative value provided by the user.
So here, I think we'd say if (timeout < TimeSpan.Zero && timeout != Timeout.InfiniteTimeSpan) and below, we change the while loop condition to be while (!cancellationToken.IsCancellationRequested) (removing the timeout check).
There was a problem hiding this comment.
I like the compromise of explicitly checking for Timeout.InfiniteTimeSpan, and erroring out in other negative TimeSpan cases. My latest commit tries to do just that.
My if statement is as you suggested, @cgillum. However, the while loop does preserve the timeout check because I think we need it for the "happy case" where the timeout is positive. I suppose I could also move it to an if-statement check inside the while loop though.
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
Outdated
Show resolved
Hide resolved
Co-authored-by: Chris Gillum <cgillum@microsoft.com>
|
@cgillum: let's try to get this right all the way. Does this latest commit look good to you? |
davidmrdavid
deleted the
dajusto/check-for-negative-timespan-waitForOrchestrationAsync
branch
…2.9.6 (#252) [](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [Microsoft.Azure.WebJobs.Extensions.DurableTask](https://togithub.com/Azure/azure-functions-durable-extension) | nuget | patch | `2.9.5` -> `2.9.6` | --- ### Release Notes <details> <summary>Azure/azure-functions-durable-extension</summary> ### [`v2.9.6`](https://togithub.com/Azure/azure-functions-durable-extension/releases/tag/v2.9.6): Durable Functions v2.9.6 https://www.nuget.org/packages/Microsoft.Azure.WebJobs.Extensions.DurableTask/2.9.6 ### Bug fixes - DT.AzureStorage: Add validation check for the timeout in WaitForOrchestrationAsync ([https://github.com/Azure/durabletask/pull/910](https://togithub.com/Azure/durabletask/pull/910)) - Fixed issue of missing dedupe states in the gRPC server client when scheduling a new orchestration via IOrchestrationServiceClient ([#​2489](https://togithub.com/Azure/azure-functions-durable-extension/issues/2489)) - Improved GRPC exception type and message when creating duplicate instances ([#​2493](https://togithub.com/Azure/azure-functions-durable-extension/issues/2493)) ### Dependency Updates - [DurableTask.AzureStorage to v1.13.8](https://togithub.com/Azure/durabletask/releases/tag/durabletask.azurestorage-v1.13.8) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/wsugarman/durabletask-azurestorage-scaler). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNS4xMzEuMCIsInVwZGF0ZWRJblZlciI6IjM1LjEzMS4wIiwidGFyZ2V0QnJhbmNoIjoibWFpbiJ9--> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
](https://renovatebot.com){kind=link}
This PR validates that the timeout in WaitForOrchestrationAsync is positive. Otherwise, we risk accidentally returning
nullwithout doing any meaningful work.This PR also enables nullable analysis for part of the
AzureStorageOrchestrationServicefileI encountered this in the process of repro'ing this issue: microsoft/durabletask-dotnet#143