Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate that timeout is positive in WaitForOrchestrationAsync, and enable nullable checks #910

Merged
merged 7 commits into from
Jun 2, 2023
Merged

Validate that timeout is positive in WaitForOrchestrationAsync, and enable nullable checks #910

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ec2bb43
enable nullable checks and check that timeout is positive in WaitForI…
davidmrdavid May 26, 2023
cf97739
allow negative timeouts
davidmrdavid Jun 1, 2023
ecd3762
explicitly check for infiniteTimeSpan
davidmrdavid Jun 2, 2023
8ca300c
Apply suggestions from code review
davidmrdavid Jun 2, 2023
26cfbd6
refactor to support TimeSpan.Zero
davidmrdavid Jun 2, 2023
76e279d
update param description
davidmrdavid Jun 2, 2023
c227c2c
check status after timeout, and refactor loop
davidmrdavid Jun 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 18 additions & 8 deletions src/DurableTask.AzureStorage/AzureStorageOrchestrationService.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
// See the License for the specific language governing permissions and
// limitations under the License.
// ----------------------------------------------------------------------------------

namespace DurableTask.AzureStorage
{
using System;
Expand Down Expand Up @@ -1838,15 +1837,15 @@ async Task<PurgeResult> IOrchestrationServicePurgeClient.PurgeInstanceStateAsync
purgeInstanceFilter.RuntimeStatus);
return storagePurgeHistoryResult.ToCorePurgeHistoryResult();
}

#nullable enable
/// <summary>
/// Wait for an orchestration to reach any terminal state within the given timeout
/// </summary>
/// <param name="instanceId">The orchestration instance to wait for.</param>
/// <param name="executionId">The execution ID (generation) of the specified instance.</param>
/// <param name="timeout">Max timeout to wait.</param>
/// <param name="timeout">Max timeout to wait. Only positive values are allowed</param>
/// <param name="cancellationToken">Task cancellation token.</param>
public async Task<OrchestrationState> WaitForOrchestrationAsync(
public async Task<OrchestrationState?> WaitForOrchestrationAsync(
string instanceId,
string executionId,
TimeSpan timeout,
Expand All @@ -1857,6 +1856,13 @@ public async Task<OrchestrationState> WaitForOrchestrationAsync(
throw new ArgumentException(nameof(instanceId));
}

if (timeout < TimeSpan.Zero)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we accept negative values? There seems to be a precedence that negative implies infinite for timeouts. Especially since Timeout.InfiniteTimeSpan is negative.

Copy link
Collaborator Author

@davidmrdavid davidmrdavid May 26, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be a better experience to accept negative as an infinite timeout.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
throw new ArgumentException($"The parameter {nameof(timeout)} cannot be negative." +
$" The value for {nameof(timeout)} was '{timeout}'." +
$" Please provide a positive timeout value.");
}

TimeSpan statusPollingInterval = TimeSpan.FromSeconds(2);
while (!cancellationToken.IsCancellationRequested && timeout > TimeSpan.Zero)
{
Expand Down Expand Up @@ -1909,7 +1915,7 @@ public Task<string> DownloadBlobAsync(string blobUri)

// TODO: Change this to a sticky assignment so that partition count changes can
// be supported: https://github.com/Azure/azure-functions-durable-extension/issues/1
async Task<ControlQueue> GetControlQueueAsync(string instanceId)
async Task<ControlQueue?> GetControlQueueAsync(string instanceId)
{
uint partitionIndex = Fnv1aHashHelper.ComputeHash(instanceId) % (uint)this.settings.PartitionCount;
string queueName = GetControlQueueName(this.settings.TaskHubName, (int)partitionIndex);
Expand Down Expand Up @@ -1980,12 +1986,12 @@ private static OrchestrationQueryResult ConvertFrom(DurableStatusQueryResult sta

class PendingMessageBatch
{
public string OrchestrationInstanceId { get; set; }
public string OrchestrationExecutionId { get; set; }
public string? OrchestrationInstanceId { get; set; }
public string? OrchestrationExecutionId { get; set; }

public List<MessageData> Messages { get; set; } = new List<MessageData>();

public OrchestrationRuntimeState Orchestrationstate { get; set; }
public OrchestrationRuntimeState? Orchestrationstate { get; set; }
}

class ResettableLazy<T>
Expand All @@ -1995,7 +2001,10 @@ class ResettableLazy<T>

Lazy<T> lazy;

// Supress warning because it's incorrect: the lazy variable is initialized in the constructor, in the `Reset()` method
#pragma warning disable CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
public ResettableLazy(Func<T> valueFactory, LazyThreadSafetyMode mode)
#pragma warning restore CS8618 // Non-nullable field must contain a non-null value when exiting constructor. Consider declaring as nullable.
{
this.valueFactory = valueFactory;
this.threadSafetyMode = mode;
Expand Down Expand Up @@ -2025,3 +2034,4 @@ public TaskHubQueueMessage(TaskHubQueue queue, TaskMessage message)
}
}
}
#nullable disable