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

Make run_until_stalled handle self-waking futures #2593

Merged
merged 5 commits into from May 11, 2022

Conversation

khollbach
Copy link
Contributor

This fixes a bug in LocalPool's run_until_stalled and try_run_one, where self-waking futures cause these methods to exit early.

The issue occurs when a future "yields" to the executor by simply waking its waker and returning Pending. As of #2551, when an underlying future "yields" in this way, the containing FuturesUnordered will do the same. Then, when this happens, the containing LocalPool assumes that this Pending means that the pool can't make any more progress.

repro

This playground, where the output is:

`Pending(0)` polled!
`Pending(1)` polled!

But it should be:

`Pending(0)` polled!
`Pending(1)` polled!
`Pending(2)` polled!
`Ready` polled!

I believe this PR should fix it, but I'm new to open source (and async Rust) so feedback is welcome!

CC'ing @gmorenz who helped me track down the cause of the bug -- thanks Greg :)

@khollbach khollbach requested a review from taiki-e as a code owner April 21, 2022 22:02
Copy link
Member

@taiki-e taiki-e left a comment

Choose a reason for hiding this comment

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

Thanks! Could you add a test case for this?

@taiki-e taiki-e added A-executor Area: futures::executor 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Apr 30, 2022
LocalPool::try_run_one and run_until_stalled now correctly re-try when a
future "yields" by calling wake and returning Pending.
@khollbach khollbach force-pushed the run_until_stalled-vs-self-waking branch from 245842c to 99dd3c8 Compare May 11, 2022 18:44
khollbach and others added 2 commits May 11, 2022 16:23
Co-authored-by: Taiki Endo <te316e89@gmail.com>
@taiki-e taiki-e merged commit 657264d into rust-lang:master May 11, 2022
taiki-e pushed a commit that referenced this pull request Aug 14, 2022
LocalPool::try_run_one and run_until_stalled now correctly re-try when a
future "yields" by calling wake and returning Pending.
@taiki-e taiki-e mentioned this pull request Aug 14, 2022
@taiki-e taiki-e added 0.3-backport: completed and removed 0.3-backport: pending The maintainer accepted to backport this to the 0.3 branch, but backport has not been done yet. labels Aug 14, 2022
taiki-e pushed a commit that referenced this pull request Aug 14, 2022
LocalPool::try_run_one and run_until_stalled now correctly re-try when a
future "yields" by calling wake and returning Pending.
@taiki-e taiki-e mentioned this pull request Aug 14, 2022
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Aug 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [futures](https://github.com/rust-lang/futures-rs) | dependencies | patch | `0.3.21` -> `0.3.23` |

---

### Release Notes

<details>
<summary>rust-lang/futures-rs</summary>

### [`v0.3.23`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0323---2022-08-14)

[Compare Source](rust-lang/futures-rs@0.3.22...0.3.23)

-   Work around MSRV increase due to a cargo bug.

### [`v0.3.22`](https://github.com/rust-lang/futures-rs/blob/HEAD/CHANGELOG.md#&#8203;0322---2022-08-14)

[Compare Source](rust-lang/futures-rs@0.3.21...0.3.22)

-   Fix `Sync` impl of `BiLockGuard` ([#&#8203;2570](rust-lang/futures-rs#2570))
-   Fix partial iteration in `FuturesUnordered` ([#&#8203;2574](rust-lang/futures-rs#2574))
-   Fix false detection of inner panics in `Shared` ([#&#8203;2576](rust-lang/futures-rs#2576))
-   Add `Mutex::lock_owned` and `Mutex::try_lock_owned` ([#&#8203;2571](rust-lang/futures-rs#2571))
-   Add `io::copy_buf_abortable` ([#&#8203;2507](rust-lang/futures-rs#2507))
-   Remove `Unpin` bound from `TryStreamExt::into_async_read` ([#&#8203;2599](rust-lang/futures-rs#2599))
-   Make `run_until_stalled` handle self-waking futures ([#&#8203;2593](rust-lang/futures-rs#2593))
-   Use `FuturesOrdered` in `try_join_all` ([#&#8203;2556](rust-lang/futures-rs#2556))
-   Fix orderings in `LocalPool` waker ([#&#8203;2608](rust-lang/futures-rs#2608))
-   Fix `stream::Chunk` adapters size hints ([#&#8203;2611](rust-lang/futures-rs#2611))
-   Add `push_front` and `push_back` to `FuturesOrdered` ([#&#8203;2591](rust-lang/futures-rs#2591))
-   Deprecate `FuturesOrdered::push` in favor of `FuturesOrdered::push_back` ([#&#8203;2591](rust-lang/futures-rs#2591))
-   Performance improvements ([#&#8203;2583](rust-lang/futures-rs#2583), [#&#8203;2626](rust-lang/futures-rs#2626))
-   Documentation improvements ([#&#8203;2579](rust-lang/futures-rs#2579), [#&#8203;2604](rust-lang/futures-rs#2604), [#&#8203;2613](rust-lang/futures-rs#2613))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4xNTYuMSIsInVwZGF0ZWRJblZlciI6IjMyLjE1Ni4xIn0=-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1507
Reviewed-by: crapStone <crapstone@noreply.codeberg.org>
Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants