Use deterministic combinators for OrchestrationContext::join/select - #19
Merged
Conversation
Replace OrchestrationContext::join's dependency on futures::future::join_all with a local poll-all future that polls every pending child on each replay poll. This avoids the upstream join_all large-fan-in path that switches to FuturesOrdered/FuturesUnordered and can rely on child wake notifications that the replay engine intentionally does not drive. Move the orchestration combinator machinery into src/combinators.rs and route ctx.join through PollAllJoin. Also add local PollAllJoin2 and PollAllJoin3 implementations for ctx.join2 and ctx.join3, and local biased Select2 and Select3 futures for ctx.select2 and ctx.select3. Updating join2, join3, select2, and select3 is not required for the current large fan-in regression, but it protects Duroxide from future upstream changes in those futures combinator implementations and keeps all orchestration concurrency under the replay engine's deterministic polling contract. Make the futures crate optional for the main crate and enable it only for provider-test, where provider validation code still uses futures helpers outside orchestration replay. Update orchestration docs and implementation notes so they describe Duroxide's replay-safe local combinators instead of futures::join_all or futures::select_biased. Verified with nextest filters covering select2, select3, join, join2, join3, select loser cancellation, and the 1024-child fan-in replay regression.
qbradley
changed the title
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
3 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.
Mark Wotton identified the bug that OrchestrationContext::join can fail to complete and provided a regression test in #18.
The root cause of the bug is that futures::future::join_all switches from a "poll all in order" strategy to using FuturesOrdered when the number of futures exceeds a threshold (currently 30 futures). See https://docs.rs/futures/latest/futures/future/fn.join_all.html#see-also.
This PR fixes the bug by replacing futures::future::join_all with a custom combinator, that always polls all pending futures in order, ensuring deterministic ordering.
The join2, join3, select2, and select3 are also updated to use custom combinators. Although currently this is not necessary, it ensures upstream changes to futures crate do not regress duroxide.
Stable Rust now has a noop waker in the standard library, so poll_once is updated to use it, removing
unsafe.The futures crate is disabled by default to discourage its use in the duroxide implementation, but still used by provider-test.
This PR incorporates Mark's regression test.