Allow consumers to implement POSIX AIO sources. #4054

asomers · 2021-08-23T01:11:59Z

Unlike every other kqueue event source, POSIX AIO registers events not
via kevent(2) but by a different mechanism that needs the kqueue's file
descriptor. This commit adds a new PollAio type, designed to be used
by an external crate that implements a POSIX AIO Future for use with
Tokio's reactor.

Fixes: #3197

Motivation

With Tokio 0.1 and 0.2, PollEvented was sufficiently powerful to allow external crates to implement a POSIX AIO event source. Beginning with Tokio 0.3, PollEvented became private, and it also lost some needed functionality. Thus, there was no possible way for a consumer to use POSIX AIO with Tokio.

Solution

The first proposed solution was to reexpose PollEvented. That idea was rejected because Tokio's maintainers thought that PollEvented peered too intimately into Tokio's internals, and they wanted a more stable external API that wasn't tied to mio's implementation.

The second solution was made in PR #3841. It created a new type, PollAio that worked much like PollEvented but crippled so that it would only be useful with POSIX AIO. However, its public API did include the mio::event::Source trait, so Tokio's maintainers rejected it because it still exposed mio data types. This PR preserves that solution as a discrete commit.

This PR introduces a third solution. It builds on the previous one, but instead of using mio::event::Source in the public API, it creates a new trait: AioSource. The new trait is very similar to mio::event::Source, but stripped down.

The new solution's advantages are that:

It's smaller than mio::event::Source
It's controlled by Tokio, not by mio, which may ease Tokio's next mio upgrade.

The new solution's disadvantages are that:

Seen in the perspective of the overall stack, AioSource is an unhelpful anti-abstraction. mio_aio::AioCb already implements mio::event::Source. But now tokio_file must implement AioSource on top of that, only for Tokio to internally reimplement mio::event::Source again. At least the runtime impact is likely to be tiny.
It requires more code upstream (asomers/mio-aio@2f56696)
It requires more code downstream (asomers/tokio-file@8aa8c1d)
AioSource deabstracts mio_aio::AioCb and mio_aio::LioCb. Previously in solution 2 those could be passed to Tokio in exactly the same way. Now, Tokio's consumers must separately implement AioSource for both. See the tests in this PR.
By using RawFd and usize in the public API instead of Registry and Token it conceals information, reducing the ability to detect bugs at compile time, though not technically unsafe.

Unlike every other kqueue event source, POSIX AIO registers events not via kevent(2) but by a different mechanism that needs the kqueue's file descriptor. This commit adds a new `PollAio` type, designed to be used by an external crate that implements a POSIX AIO Future for use with Tokio's reactor. Fixes: tokio-rs#3197

@Darksonn

Per @Darksonn Tokio has a policy of never exposing mio data types in its public API. This change creates a new AioSource trait that is similar to mio::event::Source but stripped down to work only for POSIX AIO.

asomers · 2021-08-23T01:12:21Z

cc @Darksonn

tokio/src/io/poll_aio.rs

tokio/tests/io_poll_aio.rs

tokio/src/io/poll_aio.rs

Darksonn · 2021-08-23T06:44:20Z

tokio/tests/io_poll_aio.rs

+mod aio {
+    use super::*;
+
+    /// aio_fsync is the simplest AIO operation.  This test ensures that Tokio
+    /// can register an aiocb, and set its readiness.


For both tests, I'd like to see something that performs more than one operation to verify that the readiness flag behaves properly.

For non-lio-listio operations there really isn't any such thing as performing more than one operation. The kernel forgets about the aiocb as soon as you call aio_return to get its result. You could change some fields and resubmit it, but that's equivalent to creating a new aiocb.

For lio_listio, it's hard to write a small test that requires clear_readiness. The only way that would ever be required would be due to a race or a system resource limitation. Here's a test that deliberately hits a system resource limitation
https://github.com/asomers/tokio-file/blob/master/tests/lio_listio_incomplete.rs

If you already have a test for it, it would be nice to include it in Tokio as well.

Ok, but it will take a while to adapt, to make it run without all of the other stuff in tokio-file.

I think it's important to have at least one test that includes a call to clear_readiness.

Ok, here are the tests you asked for.

The original version of these tests used a futures_test::task::noop_context so it could manually poll the futures. But it still constructed a Runtime, because PollAio requires a Runtime for its construction. This caused the Runtime to poll the future in a tight loop after event notification. Remove noop_context, implement Future, and use a normal #[tokio::test] instead.

Also, document clear_ready's uses more accurately

tokio/Cargo.toml

Darksonn · 2021-08-27T08:51:33Z

tokio/tests/io_poll_aio.rs

+    ///
+    /// An example bypassing mio_aio and Nix to demonstrate how the kevent
+    /// registration actually works, under the hood.
+    struct LlSource(Pin<Box<libc::aiocb>>);


The libc::aiocb type is Unpin so pinning it doesn't do anything.

It is actually a kernel requirement, not a Futures requirement, that the libc::aiocb have a stable address, because the kernel uses that address to identify it. Nix gets around the Unpin problem by adding a std::marker::PhantomPinned field. I could do that here too, though it isn't strictly necessary to test Tokio's functionality.

Putting it in a box is enough here, but I'm also ok with keeping the Pin as it still does serve to document your intent. You may find this interesting.

tokio/src/io/poll_aio.rs

@Darksonn

Per @Darksonn, even though these things aren't really related, they may as well use the same feature flag because they both pull in the same mio dependencies.

Darksonn

The suggestions below will make PollAio show up in the docs.rs documentation for Tokio, which is built on Linux rather than FreeBSD. Feel free to tweak the details.

Darksonn · 2021-08-31T15:00:38Z

tokio/src/macros/cfg.rs

+macro_rules! cfg_aio {
+    ($($item:item)*) => {
+        $(
+            #[cfg(all(target_os = "freebsd", feature = "net"))]


Suggested change

#[cfg(all(target_os = "freebsd", feature = "net"))]

#[cfg(any(docsrs, all(target_os = "freebsd", feature = "net")))]

tokio/src/io/driver/interest.rs

tokio/src/io/mod.rs

* export PollAio from tokio::io::poll_aio instead of tokio::io, to be more consistent with how AsyncFd does it. * build the docs for PollAio when building docs for any OS.

It passed on my system. Not sure why it failed in CI.

asomers · 2021-09-02T20:42:45Z

@Darksonn anything else you'd like to see here?

Darksonn · 2021-09-02T21:05:09Z

There will probably also be some other people taking a look, but I don't have anything else.

carllerche

Looks good. I think it is almost there and left a few smaller naming / API comments.

@Darksonn if you disagree w/ any of these comments, feel free to push back.

tokio/src/io/mod.rs

tokio/src/io/poll_aio.rs

carllerche · 2021-09-03T18:06:03Z

tokio/src/io/poll_aio.rs

+    /// is scheduled to receive a wakeup when the underlying operation
+    /// completes. Note that on multiple calls to poll, only the Waker from the
+    /// Context passed to the most recent call is scheduled to receive a wakeup.
+    pub fn poll<'a>(&'a self, cx: &mut Context<'_>) -> Poll<io::Result<PollAioEvent>> {


We avoid inherent fns named just poll as they would conflict w/ Future. Also, why is this an inherent fn? If it were implemented as a future, then it could be used with .await.

Perhaps, the inherent fn could be named poll_ready (or something) and an equivalent async fn is added async fn ready(&self) -> io::Result<AioEvent>.

Under the principle that "futures should do nothing until polled", I elected to submit the operation to the kernel (aio_write, etc) as part of poll. But that means that it needs to be in the external crate instead of in Tokio. Hence the inherent function. I'll rename it to poll_ready.

tokio/src/io/poll_aio.rs

tokio/src/io/mod.rs

asomers · 2021-09-04T01:35:02Z

The test failure is not the result of this PR.

tokio/src/io/bsd/poll_aio.rs

Co-authored-by: Alice Ryhl <[email protected]>

Darksonn

LGTM. There are a few minor details below, but nothing blocking.

tokio/src/io/bsd/poll_aio.rs

tokio/src/io/driver/interest.rs

tokio/Cargo.toml

Spelling and formatting in the comments, plus remove the trait bound from Aio's struct definition. Co-authored-by: Alice Ryhl <[email protected]>

carllerche

Thanks for sticking around and getting this through 👍

asomers · 2021-09-15T17:06:54Z

Thanks guys!

BTW, just this morning I learned of a new use case for POSIX AIO on sockets. I think it should work fine with the interface in this PR, though. I may add a new test case for it.

Darksonn · 2021-09-15T17:23:33Z

Sure, go ahead and open a PR with the extra test case.

asomers added 2 commits August 22, 2021 18:44

Remove mio data types from the PollAio API

0536be4

Per @Darksonn Tokio has a policy of never exposing mio data types in its public API. This change creates a new AioSource trait that is similar to mio::event::Source but stripped down to work only for POSIX AIO.

Appease rustfmt

5575468

asomers force-pushed the aio4 branch from 4743978 to 5575468 Compare August 23, 2021 01:50

Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Aug 23, 2021

Darksonn reviewed Aug 23, 2021

View reviewed changes

tokio/src/io/poll_aio.rs Outdated Show resolved Hide resolved

tokio/src/io/poll_aio.rs Outdated Show resolved Hide resolved

tokio/tests/io_poll_aio.rs Outdated Show resolved Hide resolved

tokio/src/io/poll_aio.rs Outdated Show resolved Hide resolved

Darksonn reviewed Aug 23, 2021

View reviewed changes

asomers added 3 commits August 26, 2021 20:53

Add a PollAio test that uses libc directly

28691c5

Add tests for PollAio::clear_ready

13a2c11

Also, document clear_ready's uses more accurately

Darksonn reviewed Aug 27, 2021

View reviewed changes

tokio/src/io/poll_aio.rs Outdated Show resolved Hide resolved

asomers added 3 commits August 27, 2021 08:09

Improve documentation for PollAio::poll

42c92db

Include aio functionality under the "net" feature flag

b2fc028

Per @Darksonn, even though these things aren't really related, they may as well use the same feature flag because they both pull in the same mio dependencies.

Slightly elaborate docs for PollAio::poll

408a9c6

Darksonn reviewed Aug 31, 2021

View reviewed changes

asomers added 2 commits August 31, 2021 10:24

Rearrange modules and docs a bit

1f70110

* export PollAio from tokio::io::poll_aio instead of tokio::io, to be more consistent with how AsyncFd does it. * build the docs for PollAio when building docs for any OS.

Maybe fix the docs build for Linux

eed4635

It passed on my system. Not sure why it failed in CI.

carllerche requested changes Sep 3, 2021

View reviewed changes

asomers added 2 commits September 3, 2021 12:55

Rename aio-related types as suggested by carllerche

7e66ce2

Satisfy rustfmt

a5e6f32

Darksonn reviewed Sep 3, 2021

View reviewed changes

tokio/src/io/mod.rs Outdated Show resolved Hide resolved

asomers added 2 commits September 3, 2021 14:28

export Aio and friends from tokio::io::bsd

69e69be

Rename Aio::poll to Aio::poll_ready

4dfa047

Darksonn reviewed Sep 4, 2021

View reviewed changes

tokio/src/io/bsd/poll_aio.rs Outdated Show resolved Hide resolved

Darksonn reviewed Sep 4, 2021

View reviewed changes

tokio/src/io/bsd/poll_aio.rs Outdated Show resolved Hide resolved

Fix doc links to poll_ready

668e725

Co-authored-by: Alice Ryhl <[email protected]>

asomers requested a review from carllerche September 4, 2021 21:58

Darksonn approved these changes Sep 7, 2021

View reviewed changes

asomers and others added 2 commits September 7, 2021 08:40

Apply darksonn's suggestions from code review

3cfb77b

Spelling and formatting in the comments, plus remove the trait bound from Aio's struct definition. Co-authored-by: Alice Ryhl <[email protected]>

Reorder Aio's methods, as suggested by @Darksonn .

b5530e8

asomers requested a review from Darksonn September 7, 2021 14:46

Darksonn approved these changes Sep 7, 2021

View reviewed changes

carllerche approved these changes Sep 15, 2021

View reviewed changes

Darksonn merged commit 8b298d9 into tokio-rs:master Sep 15, 2021

asomers deleted the aio4 branch September 15, 2021 17:05

Darksonn mentioned this pull request Sep 21, 2021

Prepare Tokio v1.12.0 #4123

Merged

softdevca mentioned this pull request Sep 21, 2021

Bump tokio from 1.9.0 to 1.12.0 softdevca/mootranscode#35

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow consumers to implement POSIX AIO sources. #4054

Allow consumers to implement POSIX AIO sources. #4054

asomers commented Aug 23, 2021

asomers commented Aug 23, 2021

Darksonn Aug 23, 2021

asomers Aug 23, 2021

Darksonn Aug 23, 2021

asomers Aug 23, 2021

Darksonn Aug 24, 2021

asomers Aug 27, 2021

Darksonn Aug 27, 2021

asomers Aug 27, 2021

Darksonn Aug 27, 2021

Darksonn left a comment

Darksonn Aug 31, 2021

asomers commented Sep 2, 2021

Darksonn commented Sep 2, 2021

carllerche left a comment

carllerche Sep 3, 2021

asomers Sep 3, 2021

asomers commented Sep 4, 2021

Darksonn left a comment

carllerche left a comment

asomers commented Sep 15, 2021

Darksonn commented Sep 15, 2021

	#[cfg(all(target_os = "freebsd", feature = "net"))]
	#[cfg(any(docsrs, all(target_os = "freebsd", feature = "net")))]

Allow consumers to implement POSIX AIO sources. #4054

Allow consumers to implement POSIX AIO sources. #4054

Conversation

asomers commented Aug 23, 2021

Motivation

Solution

asomers commented Aug 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Darksonn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Sep 2, 2021

Darksonn commented Sep 2, 2021

carllerche left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asomers commented Sep 4, 2021

Darksonn left a comment

Choose a reason for hiding this comment

carllerche left a comment

Choose a reason for hiding this comment

asomers commented Sep 15, 2021

Darksonn commented Sep 15, 2021