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

io: remove mio from the public API. #2728

Closed
carllerche opened this issue Jul 30, 2020 · 9 comments
Closed

io: remove mio from the public API. #2728

carllerche opened this issue Jul 30, 2020 · 9 comments
Labels
A-tokio Area: The main tokio crate C-proposal Category: a proposal and request for comments M-io Module: tokio/io
Milestone

Comments

@carllerche
Copy link
Member

Summary

Remove mio types from the Tokio public API. In practice, this means removing Registration and PollEvented. Some users currently depend on these types in order to integrate custom resources with Tokio. Instead of providing hook points based on mio, we would provide platform-specific "low level" resource types. For example, on *nix systems, we could provide an AsyncFd type that provides access to readiness notifications.

Motivation

Tokio cannot include public API dependencies on third party crates that do not match Tokio's stability guarantees. The Mio crate is not yet at a 1.0 release. We would like to be able to release Tokio 1.0 without blocking on Mio for 1.0. Additionally, we may want to change the low-level implementation details. For example, we may wish to switch the underlying implementation to an io_uring based backend or we may wish to provide windows integration directly instead of using the Mio compatibility layer.

Given the open questions, the safest path forward is to decouple Tokio from Mio at this point in time.

Proposal

The PollEvented and Registration types are made private. In order to provide the ability to integrate with Tokio, platform-specific low-level resource types are provided. On *nix platforms, an AsyncFd type is provided.

struct AsyncFd { ... }

impl AsyncFd {
    // Create a new `AsyncFd`. This registers the FD w/ the io-driver with the
    // given `Interest`.
    pub fn new(fd: RawFd, interest: Interest) -> AsyncFd;

    // Awaits for a readiness event covering any of the given `Interest`. The
    // `Interest` provided here must be a subset of the `Interest` supplied to
    // `new`.
    pub async fn readiness(&self, interest: Interest) -> io::Result<Readiness>;
}

struct Interest(mio::Interest);

struct Readiness(mio::Readiness);

Usage would look something like this:

let interest = Interest::READABLE
    .add(Interest::WRITEABLE);

let my_fd = AsyncFd::new(fd, interest);

// Await only *read* interest
my_fd.readiness(Interest::READABLE).await;

This would only provide extensibility on *nix platforms. Extending Windows platforms would require a completely different API. As of now, Mio does not have a strategy for extending windows platforms. We would need to wait until one is developed.

@carllerche carllerche added C-proposal Category: a proposal and request for comments A-tokio Area: The main tokio crate M-io Module: tokio/io labels Jul 30, 2020
@carllerche carllerche added this to the v0.3 milestone Jul 30, 2020
@carllerche carllerche mentioned this issue Jul 30, 2020
10 tasks
@Diggsey
Copy link
Contributor

Diggsey commented Jul 31, 2020

Another option would be to move those types into a separate crate tokio_mio_interop or something. (Export them from tokio 1.0 under a hidden module, and then have that crate re-export them publically).

@carllerche
Copy link
Member Author

I believe you are referring to Interest and Readiness here. This is true and worth considering.

The biggest impact of this proposal would be that one would no longer be able to take a T: mio::Evented and make it work with Tokio. This means that any existing Mio based "resource" type could not be made to work with Tokio.

I believe that this will cause a bit of short/medium term turmoil but the AsyncFd is a better long term strategy.

@simonbuchan
Copy link

I really would like "proper" windows async, that is some form of overlapped file IO, rather than threadpooled sync IO. IPC with named pipes in particular basically requires overlapped file IO, and I found it very hard to wrap manually wrapped file APIs so that they could be integrated with other tokio async APIs - my current code feels incredibly fragile.

Not sure what that would look like if you really want to be extensible in an AsyncFd style, maybe - AsyncRawHandle that is an fd on unix, and a IOCP-compatible HANDLE on windows?

carllerche pushed a commit that referenced this issue Sep 23, 2020
This refactors I/O registration in a few ways:

- Cleans up the cached readiness in `PollEvented`. This cache used to
  be helpful when readiness was a linked list of `*mut Node`s in
  `Registration`. Previous refactors have turned `Registration` into just
  an `AtomicUsize` holding the current readiness, so the cache is just
  extra work and complexity. Gone.
- Polling the `Registration` for readiness now gives a `ReadyEvent`,
  which includes the driver tick. This event must be passed back into
  `clear_readiness`, so that the readiness is only cleared from `Registration`
  if the tick hasn't changed. Previously, it was possible to clear the
  readiness even though another thread had *just* polled the driver and
  found the socket ready again.
- Registration now also contains an `async fn readiness`, which stores
  wakers in an instrusive linked list. This allows an unbounded number
  of tasks to register for readiness (previously, only 1 per direction (read
  and write)). By using the intrusive linked list, there is no concern of
  leaking the storage of the wakers, since they are stored inside the `async fn`
  and released when the future is dropped.
- Registration retains a `poll_readiness(Direction)` method, to support
  `AsyncRead` and `AsyncWrite`. They aren't able to use `async fn`s, and
  so there are 2 reserved slots for those methods.
- IO types where it makes sense to have multiple tasks waiting on them
  now take advantage of this new `async fn readiness`, such as `UdpSocket`
  and `UnixDatagram`.

Additionally, this makes the `io-driver` "feature" internal-only (no longer
documented, not part of public API), and adds a second internal-only
feature, `io-readiness`, to group together linked list part of registration
that is only used by some of the IO types.

After a bit of discussion, changing stream-based transports (like
`TcpStream`) to have `async fn read(&self)` is punted, since that
is likely too easy of a footgun to activate.

Refs: #2779, #2728
@bdonlan
Copy link
Contributor

bdonlan commented Oct 1, 2020

I've started work on this issue. A couple thoughts:

  1. Should AsyncFd::new() be unsafe? While it's not immediately obvious whether there are memory safety issues here, passing an unowned file descriptor to AsyncFd, closing it, and opening a new fd with the same file descriptor number could result in behavior that is undefined, with respect to tokio's API contract (for analogous reasons, std::os::unix::io::FromRawFd::from_raw_fd is unsafe)
  2. Would it be a more ergonomic API to take a generic argument implementing AsRawFd to allow for safer RAII patterns here? That is, this would discourage (but not entirely prevent) problems like closing the fd while it is registered or multiple registrations of the same fd.

@carllerche
Copy link
Member Author

  1. I don't think it needs to be unsafe. I don't believe there is any actual unsafety w.r.t working with an FD. I believe it is left over from early rust days when "unsafe" was used for things that were not strictly memory unsafe.

  2. I don't think AsyncFd should close the FD or anything like that on drop. I would lean towards just taking a RawFd but don't have a strong opinion there.

bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 2, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 2, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 2, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
@seanmonstar
Copy link
Member

Relevant to the unsafety of FromRawFd:

bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 2, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
@ipetkov
Copy link
Member

ipetkov commented Oct 3, 2020

I don't think AsyncFd should close the FD or anything like that on drop. I would lean towards just taking a RawFd but don't have a strong opinion there.

Curious around the motivation against having AsyncFd take ownership and close the fd on drop? I think taking this approach will lead to awkward juggling between an AsyncFd instance and its actual owner and making sure they are dropped in the correct order.

If the contract is such that ownership of the fd is taken, it will largely do the right thing by default when dropped. We already have the IntoRawFd trait for taking ownership back in case the caller does not want to close the fd quite yet

cc @bdonlan since you're working on this :)

bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 5, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 5, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
@carllerche
Copy link
Member Author

A PR for AsyncFd has been submitted. Merging this PR is not critical for the initial 0.3 release. We can merge it in after 0.3.

To close this issue, we need to audit tokio and ensure mio types have been removed from all public APIs.

@carllerche
Copy link
Member Author

I scanned and it looks like this is done.

bdonlan pushed a commit to bdonlan/tokio that referenced this issue Oct 20, 2020
This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: tokio-rs#2728
carllerche pushed a commit that referenced this issue Oct 22, 2020
* io: Add AsyncFd

This adds AsyncFd, a unix-only structure to allow for read/writability states
to be monitored for arbitrary file descriptors.

Issue: #2728

* driver: fix shutdown notification unreliability

Previously, there was a race window in which an IO driver shutting down could
fail to notify ScheduledIo instances of this state; in particular, notification
of outstanding ScheduledIo registrations was driven by `Driver::drop`, but
registrations bypass `Driver` and go directly to a `Weak<Inner>`. The `Driver`
holds the `Arc<Inner>` keeping `Inner` alive, but it's possible that a new
handle could be registered (or a new readiness future created for an existing
handle) after the `Driver::drop` handler runs and prior to `Inner` being
dropped.

This change fixes this in two parts: First, notification of outstanding
ScheduledIo handles is pushed down into the drop method of `Inner` instead,
and, second, we add state to ScheduledIo to ensure that we remember that the IO
driver we're bound to has shut down after the initial shutdown notification, so
that subsequent readiness future registrations can immediately return (instead
of potentially blocking indefinitely).

Fixes: #2924
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 C-proposal Category: a proposal and request for comments M-io Module: tokio/io
Projects
None yet
Development

No branches or pull requests

6 participants