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 3 commits
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
29 changes: 20 additions & 9 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 `TimeSpan` values, or `Timeout.InfiniteTimeSpan`, are allowed.</param>
davidmrdavid marked this conversation as resolved.
Show resolved Hide resolved
/// <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,8 +1856,16 @@ public async Task<OrchestrationState> WaitForOrchestrationAsync(
throw new ArgumentException(nameof(instanceId));
}

bool isInfiniteTimeSpan = timeout == Timeout.InfiniteTimeSpan;
if (timeout < TimeSpan.Zero && !isInfiniteTimeSpan)
{
throw new ArgumentException($"The parameter {nameof(timeout)} cannot be negative." +
$" The value for {nameof(timeout)} was '{timeout}'." +
$" Please provide either positive timeout value, or `Timeout.InfiniteTimeSpan`.");
davidmrdavid marked this conversation as resolved.
Show resolved Hide resolved
}

TimeSpan statusPollingInterval = TimeSpan.FromSeconds(2);
while (!cancellationToken.IsCancellationRequested && timeout > TimeSpan.Zero)
while (!cancellationToken.IsCancellationRequested && (isInfiniteTimeSpan || (timeout > TimeSpan.Zero)))
davidmrdavid marked this conversation as resolved.
Show resolved Hide resolved
{
OrchestrationState state = await this.GetOrchestrationStateAsync(instanceId, executionId);
if (state == null ||
Expand Down Expand Up @@ -1909,7 +1916,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 +1987,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 +2002,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 +2035,4 @@ public TaskHubQueueMessage(TaskHubQueue queue, TaskMessage message)
}
}
}
#nullable disable