fix(cie): update checkout action and fix cancel_executor race#52
Merged
atsushi421 merged 2 commits intomainfrom Apr 22, 2026
Merged
fix(cie): update checkout action and fix cancel_executor race#52atsushi421 merged 2 commits intomainfrom
atsushi421 merged 2 commits intomainfrom
Conversation
- Update actions/checkout from v2 to v4 in run-pre-commit workflow for consistency with build-and-test workflow and security. - Fix race condition in cancel_executor where thread_initialized could be true before spin() sets spinning=true, causing cancel() to have no effect. Always wait for is_spinning() before cancelling. Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
- Add comment explaining why cancel_executor must always wait for is_spinning() to prevent re-introduction of the race condition. - Add joinable() guard to avoid undefined behavior when the thread was never started. Signed-off-by: atsushi421 <atsushi.yano.2@tier4.jp>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses CI hygiene and a concurrency bug that could deadlock executor shutdown.
Changes:
- Updated
run-pre-commit.yamlto useactions/checkout@v4(consistent with the repo’s other workflow and avoids deprecated Node 12-based checkout). - Removed the
thread_initializedfast-path incancel_executor()and now always waits forexecutor->is_spinning()before callingcancel(), preventing a race where cancellation could be overwritten. - Added a
std::thread::joinable()guard to avoid joining an unstarted thread.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
callback_isolated_executor/src/component_container_callback_isolated.cpp |
Removes thread_initialized and fixes a shutdown race by waiting for is_spinning() before cancel + join, plus adds a joinable guard. |
.github/workflows/run-pre-commit.yaml |
Bumps actions/checkout from v2 to v4. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix two issues found during repository-wide code review:
CI: Update
actions/checkoutversion —run-pre-commit.yamlwas usingactions/checkout@v2(deprecated, Node.js 12-based) whilebuild-and-test.yamlalready uses@v4. Updated to@v4for consistency and security.Fix race condition in
cancel_executor— Thethread_initializedflag was set totruebeforespin()internally setsspinning=true. Ifcancel_executorobservedthread_initialized==trueand skipped theis_spinning()wait, it could callcancel()(which setsspinning=false) beforespin()setspinning=true. The subsequentspin()would override the cancellation and run indefinitely, blockingthread.join()forever.The fix:
thread_initializedfast-path and always wait foris_spinning()before callingcancel().thread_initializedfield fromExecutorWrapperentirely.joinable()guard to avoid undefined behavior if the thread was never started (e.g.,std::threadconstructor threw).is_spinning()wait is necessary, to prevent re-introduction of the fast-path optimization.Related links
How was this PR tested?
Notes for reviewers