Fix data race and memory corruption in when_any_state::set - #64
Open
zerodefect wants to merge 2 commits into
Open
Fix data race and memory corruption in when_any_state::set#64zerodefect wants to merge 2 commits into
zerodefect wants to merge 2 commits into
Conversation
Concurrent task completions in when_any cause multiple threads to invoke when_any_state::set simultaneously. Due to C++ eager argument evaluation, std::move(result) is evaluated by multiple threads before the internal event.set() lock-free check occurs. Moving from the shared std::vector or std::tuple concurrently leads to double-frees and memory corruption. This commit introduces a wait-free std::atomic<bool> is_set guard into when_any_state. By wrapping the execution in a compare_exchange_strong block, we guarantee that only the first arriving thread evaluates the std::move and triggers the event, safely preserving memory isolation without introducing mutex overhead.
Contributor
Author
|
Attempts to resolve #63 . |
Contributor
Author
|
Hi @Amanieu , I appreciate that you have moved over to the world of Rust and probaly have you head buried elsewhere, but is maybe something you could pick up? I like to think this change is quite small, and very obvious when you see it. Thanks. |
Owner
|
I don't like this because the atomic is redundant with the one in |
When multiple monitored tasks completed simultaneously, multiple worker threads concurrently evaluated the arguments for event.set({i, std::move(result)}). Because C++ evaluates function arguments before jumping into the function body, this caused a data race on the shared result state before the internal event_task lock could be acquired.
Changes:
* Introduced set_internal_with to basic_event to execute a lambda only after the atomic lock is successfully acquired.
* Exposed set_with on all event_task specializations (Result, Result&, and void).
* Refactored when_any_state::set to use event.set_with(), wrapping the payload construction and std::move inside a lambda.
* Guaranteed that only the single winning thread constructs the payload and executes the move operation, preventing undefined behavior.
* Refactored the original set_internal to forward calls through the new lambda architecture to maintain backwards compatibility.
zerodefect
force-pushed
the
bugfix/when_any_race_condition
branch
from
September 4, 2026 09:44
69c54e2 to
8c1ef78
Compare
Contributor
Author
|
Hi @Amanieu , thanks for looking this over. I've revisited the code. I'm still testing it, so these changes should not yet be merged. Nonetheless, I'd appreciate any feedback in the interim. |
Amanieu
reviewed
Sep 4, 2026
Amanieu
left a comment
Amanieu
left a comment
Owner
There was a problem hiding this comment.
I'm out of practice with C++ so I had Codex review your changes. It pointed out 2 things:
- Lambda deduced return types will strip references, this breaks
event_task<Value&>. - Callable types should still be forwarded so the call can be invoked as an rvalue.
| bool set_with(Func&& f) | ||
| { | ||
| return this->set_internal_with([&f] { | ||
| f(); |
Owner
There was a problem hiding this comment.
Suggested change
| f(); | |
| std::forward<Func>(f)(); |
| LIBASYNC_TRY { | ||
| // Store the result and finish | ||
| get_internal_task(*this)->set_result(std::forward<T>(result)); | ||
| get_internal_task(*this)->set_result(f()); |
Owner
There was a problem hiding this comment.
Suggested change
| get_internal_task(*this)->set_result(f()); | |
| get_internal_task(*this)->set_result(std::forward<Func>(f)()); |
| template<typename T> | ||
| bool set_internal(T&& result) const | ||
| { | ||
| return set_internal_with([&] { return std::forward<T>(result); }); |
Owner
There was a problem hiding this comment.
Suggested change
| return set_internal_with([&] { return std::forward<T>(result); }); | |
| return set_internal_with([&]() -> T&& { return std::forward<T>(result); }); |
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
2 participants
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.
Concurrent task completions in when_any cause multiple threads to invoke when_any_state::set simultaneously. Due to C++ eager argument evaluation, std::move(result) is evaluated by multiple threads before the internal event.set() lock-free check occurs. Moving from the shared std::vector or std::tuple concurrently leads to double-frees and memory corruption.
This commit introduces a wait-free std::atomic is_set guard into when_any_state. By wrapping the execution in a compare_exchange_strong block, we guarantee that only the first arriving thread evaluates the std::move and triggers the event, safely preserving memory isolation without introducing mutex overhead.