Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix FuturesUnordered #105

Merged
merged 3 commits into from
May 3, 2023
Merged

Conversation

sarsko
Copy link
Contributor

@sarsko sarsko commented May 2, 2023

Currently, code using FuturesUnordered has a chance to cause Shuttle to erroneously report a deadlock.

For instance, if we take the following code:

  shuttle::future::block_on(async {
        let mut tasks = (0..100).map(|_| spawn(async move {})).collect::<FuturesUnordered<_>>();

        while let Some(result) = tasks.next().await {
            result.unwrap();
        }
    });

When we call next() on a task, that task gets dequeued from the ready_to_run_queue inside FuturesUnordered. If that task is Poll::Ready, then all is fine. If it is Poll::Pending (in the case where we call next() before the spawn has had a chance to execute) however, then the task is never polled again, as we don't wake the associated waker.

This PR seeks to solve this issue by waking the waker, which is stored together with the result in the Wrapper.

Currently the test suite is a bit lacking, and only contains three tests of common patterns with futures that don't do anything. I have some more tests locally, but they are really just reskins of existing tests, so I am not entirely sure they check any interesting behaviour. I want to add some more interesting tests, so going to look at that next. If anyone has any suggestions, then I am all ears. Also putting this out here to get feedback on the solution in general.


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

Copy link
Member

@jamesbornholt jamesbornholt left a comment

Choose a reason for hiding this comment

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

In terms of unit tests, I think the thing we're missing is polling the JoinHandle with a custom waker implementation -- things accidentally work today if the JoinHandle is polled with a waker provided by Shuttle, because it defers to the internal set_waiter mechanism. So we want a unit test that somehow validates the waker is invoked when it's supposed to be.

src/future/mod.rs Outdated Show resolved Hide resolved
src/future/mod.rs Outdated Show resolved Hide resolved
src/future/mod.rs Outdated Show resolved Hide resolved
tests/future/streams.rs Outdated Show resolved Hide resolved
src/future/mod.rs Outdated Show resolved Hide resolved
tests/future/streams.rs Outdated Show resolved Hide resolved
block_on(async {
let mut tasks = (0..100).map(|_| spawn(async move {})).collect::<FuturesUnordered<_>>();

while let Some(result) = tasks.next().await {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure this test really adds anything new, since the above collect essentially does the same thing, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, kinda, the internals of Next and Collect are a bit different. I think it is fine to have both.

src/future/mod.rs Show resolved Hide resolved
src/future/mod.rs Show resolved Hide resolved
src/future/mod.rs Outdated Show resolved Hide resolved
jorajeev
jorajeev previously approved these changes May 3, 2023
@sarsko
Copy link
Contributor Author

sarsko commented May 3, 2023

Just a heads up that a fix with the double locking fixed and a test for the custom waker is coming.

@sarsko
Copy link
Contributor Author

sarsko commented May 3, 2023

Hmm, it can be reapproved and merged now. The behaviour the test is intended to cover is currently exercised by FuturesUnordered through the existing tests. Can add the test later as we don't want to rely on FuturesUnordered internals.

@jamesbornholt jamesbornholt merged commit 259bb95 into awslabs:main May 3, 2023
akoshelev added a commit to private-attribution/ipa that referenced this pull request May 28, 2023
Need [this](awslabs/shuttle#105) fix, otherwise
our concurrency tests are failing
akoshelev added a commit to private-attribution/ipa that referenced this pull request May 28, 2023
Need [this](awslabs/shuttle#105) fix, otherwise
our concurrency tests are failing
jorajeev pushed a commit that referenced this pull request Feb 29, 2024
* Fix FuturesUnordered

* Address comments from @jamesbornholt and @jorajeev

* Remove double locking in future/mod.rs
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.

3 participants