Skip to content

fix awaitCompletion method functionality - #9381

Open
hadley31 wants to merge 1 commit into
wpilibsuite:mainfrom
hadley31:fix/await-completion
Open

fix awaitCompletion method functionality#9381
hadley31 wants to merge 1 commit into
wpilibsuite:mainfrom
hadley31:fix/await-completion

Conversation

@hadley31

@hadley31 hadley31 commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

awaitCompletion() is meant to block the caller until every command it forked has finished running (per its Javadoc: "Waits for all forked commands to complete"). The previous implementation iterates each forked command exactly once and yields at most once per command, without ever re-checking whether that command finished. So for the example in Coroutine.fork()'s Javadoc:

Command example() {
  return Command.noRequirements(coroutine -> {
    Command child = ...;
    ForkResult childTask = coroutine.fork(child);
    // ... do more things
    // then sync back up with the child command
    if (childTask.successful()) {
      childTask.awaitCompletion();
    }
  }).named("Example");
}

if child was still running at the time awaitCompletion() was called, the loop would yield exactly once and then return — regardless of whether child had actually completed. The parent would resume after a single scheduler tick, not when the child was actually done, silently breaking the "sync back up with the forked command" contract and potentially running concurrently with a child it believed had finished.

Fix

Replace the single-pass for loop with a while loop that keeps yielding as long as any forked command is still running. This correctly blocks the caller across as many ticks as needed until all forked commands have finished, matching the documented behavior.

@hadley31
hadley31 requested a review from a team as a code owner September 4, 2026 22:21
@github-actions github-actions Bot added the component: commands v3 WPILib Commands library version 3 label Sep 4, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3866b12b69

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

if (m_scheduler.isRunning(command)) {
Coroutine.this.yield();
}
while (m_forkedCommands.stream().anyMatch(m_scheduler::isRunning)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Stop tracking a fork entry after its original run ends

When one forked command completes while another remains active, this loop keeps the completed command in m_forkedCommands; if that same Command instance is subsequently scheduled again by a trigger, default binding, or direct scheduler call, isRunning() treats the new execution as part of the old fork. The parent then waits for this unrelated run and can hang indefinitely if it is persistent, contrary to the documented promise that completed forked commands are not rescheduled or awaited again. Track the original scheduling lifecycle, or permanently retire each entry once its forked execution ends.

Useful? React with 👍 / 👎.

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

Labels

component: commands v3 WPILib Commands library version 3

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant