Skip to content

[Bugfix] adding completableFuture docs and minor bug fixes#111

Merged
zhongkechen merged 2 commits into
mainfrom
phaser
Feb 24, 2026
Merged

[Bugfix] adding completableFuture docs and minor bug fixes#111
zhongkechen merged 2 commits into
mainfrom
phaser

Conversation

@zhongkechen

@zhongkechen zhongkechen commented Feb 19, 2026

Copy link
Copy Markdown
Contributor

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

Issue Link, if available

Fix #97

Follow-up actions on previous PR #103

Description

  • Added more comments to CompletableFuture related code
  • Added a new design doc ADR-003 for CompletableFuture based operation coordination
  • Removed duplicate method isTerminalStatus
  • Fixed a bug in ChildContextOperation that doesn't handle SuspendExecutionException properly, causing flaky test cases
  • Fixed minor issues with runners
    • Added handling of errors in CloudDurableTestRunner
    • Fixed LocalDurableTestRunner gets stuck when skipTimer is true
  • Replaced some magic values in test cases with constants
  • simplified thread local and active thread management
    • removed unset/reset thread local object from waitForOperationCompletion. Thread local object is set when the thread is started and it is recycled when the thread is terminated.
    • removed thread type from active thread tracker. Only thread id is needed to track active threads.
    • renamed some methods to better reflect the purpose and avoid confusion. Context is renamed to ThreadContext to reflect that it's stored in thread local.

Demo/Screenshots

Checklist

  • I have filled out every section of the PR template
  • I have thoroughly tested this change

Testing

Unit Tests

Have unit tests been written for these changes? Yes

Integration Tests

Have integration tests been written for these changes? Yes

Examples

Has a new example been added for the change? (if applicable) Yes

@zhongkechen
zhongkechen marked this pull request as ready for review February 19, 2026 23:11
@zhongkechen zhongkechen self-assigned this Feb 20, 2026
@zhongkechen zhongkechen changed the title [Improvement] enable nested async steps [Improvement] adding completableFuture docs and removing nested step restriction Feb 20, 2026
@zhongkechen zhongkechen changed the title [Improvement] adding completableFuture docs and removing nested step restriction [Improvement] adding completableFuture docs Feb 20, 2026
@zhongkechen zhongkechen changed the title [Improvement] adding completableFuture docs [Improvement] adding completableFuture docs and minor code improvements Feb 20, 2026
@zhongkechen zhongkechen changed the title [Improvement] adding completableFuture docs and minor code improvements [Bugfix] adding completableFuture docs and minor bug fixes Feb 20, 2026
fix local runner with skipTime

fix handling of SuspendExecutionException in ChildContextOperation
@zhongkechen
zhongkechen requested a review from SilanHe February 24, 2026 17:48
@zhongkechen
zhongkechen merged commit e4d19d2 into main Feb 24, 2026
10 checks passed
@zhongkechen
zhongkechen deleted the phaser branch February 24, 2026 17:50
@zhongkechen

Copy link
Copy Markdown
Contributor Author

The following is AI (GPT 5.5)'s opinion on ADR-002 Phaser vs ADR-003 CompletableFuture for reference:

I’d still choose ADR-003 / CompletableFuture for this SDK.

The short version: Durable operation coordination is fundamentally a completion problem, not a barrier synchronization problem. A step, wait, invoke, or callback eventually reaches a terminal state, and any blocked caller should resume after that terminal state is durably recorded. CompletableFuture models that directly. Phaser can be made to work, but it asks the implementation to encode lifecycle meaning into phase numbers and party counts, which is exactly the kind of cleverness that becomes fragile under replay, duplicate updates, and edge-case timing.

Why CompletableFuture is better here:

  1. The domain is one-shot completion.
    A durable operation needs to say: “I am complete; unblock anyone waiting on me.” CompletableFuture.complete(...) naturally supports that and is idempotent in spirit: duplicate completion attempts do not advance some hidden global phase. That matters because durable systems often see repeated state from replay, polling, retries, or backend responses.

  2. Phaser depends on assumptions that the system cannot always guarantee.
    ADR-003 calls this out well: the Phaser model assumes a certain number of parties and specific phase transitions. In a concurrent replay/checkpoint system, those assumptions are hard to maintain. If you under-arrive, you deadlock. If you over-arrive, you move past the intended phase. Both are nasty failure modes.

  3. CompletableFuture makes the coordination object match the operation.
    Each operation owns a future. Waiters attach to that operation. Completion happens when the operation is durably complete. That gives a cleaner mental model than “operation phase 0 means running, phase 1 means completing, phase 2 means safe to deregister,” especially for future contributors.

  4. The failure modes are easier to test and explain.
    The CompletableFuture design still has subtle ordering requirements, especially around re-registering a blocked user thread before the step thread deregisters. But that logic is localized. The current code’s synchronized block around future completion/waiter registration is easier to reason about than a multi-phase protocol spread across operation execution and checkpoint callbacks.

  5. It leaves the two-thread-pool idea intact.
    The valuable part of Philipp’s design was isolating user code from SDK/system work. CompletableFuture does not reject that; it gives the same architecture a simpler coordination primitive.

The best argument for ADR-002 is that Phaser explicitly models a multi-party handoff, and the two-phase protocol was trying to solve a real race: “don’t let the last worker deregister before the blocked caller is active again.” That is a serious problem, and the ADR-002 design shows thoughtful understanding of it. But I think Phaser is too powerful and too implicit for this case. It makes correctness depend on everyone participating in exactly the right phase choreography.

So my judgment: ADR-003 is the better long-term design, not because ADR-002 was careless, but because ADR-002 solved a hard problem with a synchronization primitive whose semantics did not quite fit durable replay. For SDK code that customers rely on, boring and directly modeled usually wins.

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.

[Refactor]: issues with Phaser

2 participants