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

rt: fix *_closed false positives #5231

Merged
merged 11 commits into from
Dec 5, 2022
Merged

Conversation

satakuma
Copy link
Member

Motivation

Readiness futures inconsistently return current readiness of a I/O resource, if it is immediately available, or all readiness relevant for the given Interest, if a future needs to wait. In particular, it always returns read_closed for Interest::READABLE and write_closed for Interest::WRITABLE, what often is not true. Tokio should not tolerate false positives for *_closed events because they are considered final states and are not cleared internally.

In case of a io_resource.ready(Interest::READABLE | Interest::WRITABLE) call, this behavior may also lead to false positives of other events.

Solution

If a Readiness future is woken up by a new event, store the relevant readiness and return it later.
It pumps the Waiter and enclosing structs a bit (8 bytes on x86_64). If it's an issue, readiness can be packed into something smaller.

Closes: #5098

@github-actions github-actions bot added the R-loom Run loom tests on this PR label Nov 24, 2022
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Nov 25, 2022
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, but I would also like a review from Carl.

@Darksonn Darksonn requested a review from carllerche November 25, 2022 08:04
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

Thanks for doing this. You are correct the current implementation is buggy. Most of this looks good. I left a comment inline.


// Add more readiness which might have appeared in the meantime.
let curr_ready = Ready::from_usize(READINESS.unpack(curr));
let ready = w.ready | (curr_ready.intersection(w.interest));
Copy link
Member

Choose a reason for hiding this comment

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

If a readiness bit is not included in curr_ready then something else already cleared it. Including w.ready here would result in additional false positives & unnecessary I/O ops.

Based on discord, it sounds like you want to avoid empty readiness events, but that seems better to me than false positives. The caller would loop and wait again.

@satakuma
Copy link
Member Author

@carllerche I applied your suggestion to return the current readiness.

I also updated the docs of every ready method to mention false-positives and empty events. My motivation is that empty Ready events were not really possible before this change. As far as I can tell from the docs, there is no explicit guarantee about no empty return values, but I can still imagine a program logic which would assume that at least one of the requested state is set. For example, code which would deduce that if !io.ready(Interest::READABLE).await?.is_readable(), then the read must be closed. For that reason I think an explicit mention in the docs will be helpful.

@satakuma satakuma requested a review from carllerche December 2, 2022 17:40
Copy link
Member

@carllerche carllerche left a comment

Choose a reason for hiding this comment

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

LGTM thanks 👍

carllerche added a commit that referenced this pull request Dec 5, 2022
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
@carllerche carllerche merged commit 644cb82 into tokio-rs:master Dec 5, 2022
carllerche added a commit that referenced this pull request Dec 5, 2022
### Fixed
 - net: fix Windows named pipe connect ([#5208])
 - io: support vectored writes for `ChildStdin` ([#5216])
 - io: fix `async fn ready()` false positive for OS-specific events ([#5231])

 ### Changed
 - runtime: `yield_now` defers task until after driver poll ([#5223])
 - runtime: reduce amount of codegen needed per spawned task ([#5213])
 - windows: replace `winapi` dependency with `windows-sys` ([#5204])

 [#5208]: #5208
 [#5216]: #5216
 [#5213]: #5213
 [#5204]: #5204
 [#5223]: #5223
 [#5231]: #5231
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Dec 23, 2022
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dependencies | minor | `1.22.0` -> `1.23.0` |
| [tokio](https://tokio.rs) ([source](https://github.com/tokio-rs/tokio)) | dev-dependencies | minor | `1.22.0` -> `1.23.0` |

---

### Release Notes

<details>
<summary>tokio-rs/tokio</summary>

### [`v1.23.0`](https://github.com/tokio-rs/tokio/releases/tag/tokio-1.23.0): Tokio v1.23.0

[Compare Source](tokio-rs/tokio@tokio-1.22.0...tokio-1.23.0)

##### Fixed

-   net: fix Windows named pipe connect ([#&#8203;5208])
-   io: support vectored writes for `ChildStdin` ([#&#8203;5216])
-   io: fix `async fn ready()` false positive for OS-specific events ([#&#8203;5231])

##### Changed

-   runtime: `yield_now` defers task until after driver poll ([#&#8203;5223])
-   runtime: reduce amount of codegen needed per spawned task ([#&#8203;5213])
-   windows: replace `winapi` dependency with `windows-sys` ([#&#8203;5204])

[#&#8203;5208]: tokio-rs/tokio#5208

[#&#8203;5216]: tokio-rs/tokio#5216

[#&#8203;5213]: tokio-rs/tokio#5213

[#&#8203;5204]: tokio-rs/tokio#5204

[#&#8203;5223]: tokio-rs/tokio#5223

[#&#8203;5231]: tokio-rs/tokio#5231

</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 these updates again.

---

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

---

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

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1687
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
A-tokio Area: The main tokio crate M-io Module: tokio/io R-loom Run loom tests on this PR
Projects
None yet
3 participants