Skip to content

UndoAsync/RedoAsync throw ArgumentOutOfRangeException after the change is applied when the navigation timeout is TimeSpan.MaxValue or negative #104

Description

@matt-edmondson

What's wrong

NavigateSafelyAsync (UndoRedo/Services/UndoRedoService.cs:269-270) is meant to never throw. Its doc comment says a failure there "would tell the caller the undo or redo did not happen when it did, and a retry would then undo a second command", and #100 fixed exactly that. But the CancellationTokenSource setup sits outside the try:

using CancellationTokenSource cts = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken);
cts.CancelAfter(_options.EffectiveNavigationTimeout);   // <- can throw, not inside try

try
{
    await _navigationProvider.NavigateToAsync(navigationContext!, cts.Token).ConfigureAwait(false);
}
catch (Exception) { }

CancelAfter(TimeSpan) throws ArgumentOutOfRangeException for any value above uint.MaxValue - 1 ms (~49.7 days) and for any negative value other than -1 ms. Nothing validates the timeout. UndoRedoOptions.DefaultNavigationTimeout and UndoRedoOptionsBuilder.WithNavigationTimeout(TimeSpan) both accept any TimeSpan, and TimeSpan.MaxValue is a natural way to say "no timeout".

Repro

var service = new UndoRedoService(/* stack/boundary managers */, navigationProvider,
    new UndoRedoOptions(DefaultNavigationTimeout: TimeSpan.MaxValue));
int v = 0;
await service.ExecuteAsync(new DelegateCommand(() => v++, () => v--, navigationContext: "ctx"));
await service.ExecuteAsync(new DelegateCommand(() => v++, () => v--, navigationContext: "ctx"));
await service.UndoAsync();
  • Observed: ArgumentOutOfRangeException (Parameter 'delay') from line 270. At that point v == 1 and CurrentPosition == 0, so the undo was already applied. A caller that retries on the exception undoes a second command.
  • Expected: UndoAsync returns true. Navigation runs with no timeout (or is skipped), and in any case it must not surface an exception.

The same applies to RedoAsync and UndoToSaveBoundaryAsync, which also go through NavigateSafelyAsync.

Suggested fix

  • Move the CreateLinkedTokenSource/CancelAfter lines inside the try, so NavigateSafelyAsync really can't throw.
  • Treat Timeout.InfiniteTimeSpan, values ≤ 0 and values above the CancelAfter limit as "no timeout" and skip CancelAfter. Alternatively, validate the value in UndoRedoOptions/WithNavigationTimeout so it fails at configuration time rather than mid-undo.
  • Add a regression test using TimeSpan.MaxValue and a negative timeout.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Labels

bugSomething isn't working

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions