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.

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

Async loading and cancellation tokens in CloudRequestEngine #137

Conversation

drasmart
Copy link
Contributor

@drasmart drasmart commented Oct 18, 2024

🚧 Expands on #134

For the content delta see:

Changes

  • Introduce TaskWaitingCancellation/TaskExtensions static class -- provides Task<T> WithCancellation<T>(this Task<T>, CancellationToken) extension method:
    • Enables cancelling the awaiting on another task.
    • The original task is not affected, await cancellation is just like detaching.
  • Introduce AsyncLazyFailable:
    • Counterpart to [Lazy<T>].
    • Value accessed via Task<TResult> GetValueAsync(CancellationToken) method.
    • At most one task (internal "active") is attempting to run value factory delegate at any time.
    • Any number of tasks can cancellably await for the active task to finish.
      • Cancelling the waiting does not cancel already-started "active" task.
    • Exceptions properly propagate (though may now be wrapped into AggregateException).
  • Update CloudRequestEngine:
    • Swap out [Lazy<T>] fields for AsyncLazyFailable.
    • Start requesting the "lazy" values (EvidenceKeyFilter, PublicProperties) at constructor time.
    • Pass IFlowData.ProcessingCancellationToken to [HttpClient.SendAsync].
  • Update IFlowData interface:
    • Add void Process(CancellationToken) method -- allows the caller to cancel the operation.
      • Correspondingly update FlowData class -- Process() now forwards to Process(CancellationToken) successor implementation.
    • Add CancellationToken ProcessingCancellationToken { get; } property -- exposes the provided [CancellationToken] to pipeline and flow elements.
      • Pipeline.Process(IFlowData) and FlowData.Process(CancellationToken) now respect ProcessingCancellationToken and can throw [OperationCanceledException].
  • Update Web integrations:
  • Update CloudRequestException:
    • Add constructors that accept responseHeaders in IReadOnlyDictionary<string, string> form -- as returned by ResponseHeaders property -- to allow re-constructing the exception from AggregateException's inner cause.
    • Move default value (empty dictionary) for ResponseHeaders from constructors lacking this parameter to the getter -- return non-null even if constructor is called with explicitly null responseHeaders parameter.
  • Update EvidenceKeyFilterWhitelist:
    • Add constructors that accept inclusionList as IEnumerable<string> rather than List<string> (or IReadOnlyDictionary<string, int> rather than Dictionary<string, int>) -- as they are never mutated and don't need to depend on specific container implementation.
  • Update EvidenceKeyFilterAggregator:
  • Add unit tests for:
    • AsyncLazyFailable implementation.
    • TaskExtensions.WithCancellation implementation.
    • CloudRequestEngine and Pipeline reacting to IFlowData processing cancellation.
  • Includes content of [cancellation-tokens-4.4] (to be merged by Suppress calls to cloud server for a Recovery Period after request failures. #134 first).

Why

Test results

Screenshots

Example Before recovery
(new stacktrace)
During recovery
(unaffected)
GettingStarted-Web Screenshot 2024-10-18 114239 Screenshot 2024-10-18 114418
Framework-Web Screenshot 2024-10-18 114537 Screenshot 2024-10-18 114603

…get }` (via `LoadAspectProperties(ICloudRequestEngine)`).
# Conflicts:
#	FiftyOne.Pipeline.CloudRequestEngine/FlowElements/CloudRequestEngine.cs
…all `IFlowData.Process(CancellationToken)` now.
…lways consumed by at least one task (i.e. never bubble up to the worker).
…lways consumed by at least one task (i.e. never bubble up to the worker). pt.2
…lways consumed by at least one task (i.e. never bubble up to the worker). pt.3
…lways consumed by at least one task (i.e. never bubble up to the worker). pt.4
@justadreamer
Copy link
Contributor

Decided to possibly postpone to version 5.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants