Skip to content

Align waitforexternalevent waiter picking order to LIFO#509

Merged
YunchuWang merged 3 commits intomainfrom
wangbill/gh
Nov 25, 2025
Merged

Align waitforexternalevent waiter picking order to LIFO#509
YunchuWang merged 3 commits intomainfrom
wangbill/gh

Conversation

@YunchuWang
Copy link
Copy Markdown
Member

@YunchuWang YunchuWang commented Nov 21, 2025

Fix #508

This pull request refactors the handling of external events in the orchestration worker to use a stack (LIFO) approach instead of a queue (FIFO). This change improves the delivery of events to the most recent waiter, making it easier for users to abandon events they no longer care about and avoids issues with cancelled waiters. The update is thoroughly validated by new and updated integration tests that confirm correct LIFO event delivery and proper handling of cancellation and buffering scenarios.

Core event handling changes:

  • Refactored the externalEventSources data structure in TaskOrchestrationContextWrapper from a queue to a stack using a single-linked list, so the most recent waiter receives events first.
  • Updated the logic in WaitForExternalEvent and CompleteExternalEvent to add new waiters to the top of the stack and deliver events in LIFO order, ensuring correct handling when waiters are cancelled or abandoned [1] [2] [3].
  • Extended the IEventSource interface and its implementation to support stack-based linking via a new Next property [1] [2].

Testing and validation:

  • Added a comprehensive test suite (ExternalEventStackTests.cs) that covers LIFO event delivery, cancellation scenarios, multiple waiters, and event buffering to ensure correctness of the new stack-based behavior.
  • Updated existing tests (e.g., OrchestrationPatterns.cs) to expect LIFO delivery order, reflecting the new semantics.

Copilot AI review requested due to automatic review settings November 21, 2025 01:20
@YunchuWang YunchuWang changed the title Align isolated waitforexternalevent waiter picking order to LIFO Align waitforexternalevent waiter picking order to LIFO Nov 21, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the external event waiter picking order from FIFO (First In First Out) to LIFO (Last In First Out) for the isolated worker implementation, aligning it with the in-process model behavior. This change makes it easier for users to abandon external events they no longer care about, particularly in scenarios using Task.WhenAny in loops.

  • Replaced Queue-based event waiter storage with a custom Stack implementation using a single-linked list
  • Added comprehensive integration tests validating LIFO behavior including cancellation scenarios, multiple waiters/events, and event buffering
  • Updated existing test expectations to reflect LIFO ordering

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File Description
src/Worker/Core/Shims/TaskOrchestrationContextWrapper.cs Refactored external event handling from Queue (FIFO) to Stack (LIFO) by changing the data structure from Dictionary<string, Queue<IEventSource>> to Dictionary<string, IEventSource> with linked list implementation, and updated event completion logic to pop from the top of the stack
src/Worker/Core/Shims/TaskOrchestrationContextWrapper.EventSource.cs Added Next property to IEventSource interface and its implementation to support single-linked list structure for LIFO ordering
test/Grpc.IntegrationTests/OrchestrationPatterns.cs Updated ExternalEventsInParallel test expectations to reflect LIFO behavior with detailed explanatory comments
test/Grpc.IntegrationTests/ExternalEventStackTests.cs Added new comprehensive integration test file with four test cases covering LIFO behavior, cancellation handling (Issue #508), multiple events/waiters, and event buffering scenarios

Copilot AI review requested due to automatic review settings November 21, 2025 14:24
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

test/Grpc.IntegrationTests/ExternalEventStackTests.cs:179

  • This assignment to metadata is useless, since its value is never read.
        await server.Client.WaitForInstanceStartAsync(
            instanceId, this.TimeoutToken);

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

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.

Breaking change in WaitForExternalEvent causes external events being dropped

4 participants