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: add get_{ref,mut} methods to AsyncFdReadyGuard and AsyncFdReadyMutGuard. #3807

Merged
merged 3 commits into from Jun 16, 2021

Conversation

brain0
Copy link
Contributor

@brain0 brain0 commented May 22, 2021

The only way to use AsyncFdReadyMutGuard currently is through try_io, since it is impossible to access the underlying object. In some situations, it is desirable to use clear_ready and retain_ready directly instead of try_io. Enable this by adding get_ref and get_mut methods.

Motivation

When using AsyncFd with an I/O object that does not communicate readiness through ErrorKind::WouldBlock, code using try_io becomes hard to read. In the following example, the next() method returns None when no new data is ready.

let mut guard = ready!(self.inner.poll_read_ready_mut(cx))?;

match guard.try_io(|s| {
    s.get_mut()
        .next()
        .ok_or_else(|| ErrorKind::WouldBlock.into())
}) {
    Ok(event) => Poll::Ready(Some(event)),
    Err(_) => Poll::Pending,
}

As seen here, you have to explicitly convert None to a WouldBlock error in try_io, only to convert that error back into Pending afterwards. This is confusing to read.

Solution

By adding AsyncFdReadyMutGuard::get_mut, this can be expressed in a more straight-forward way:

let mut guard = ready!(self.inner.poll_read_ready_mut(cx))?;

match guard.get_mut().get_mut().next() {
    Some(event) => {
        guard.retain_ready();
        Poll::Ready(Some(Ok(event)))
    }
    None => {
        guard.clear_ready();
        Poll::Pending
    }
}

The get_ref methods are added for consistency.

Questions

Should we return a reference to AsyncFd<T> or T? try_io uses AsyncFd<T> in its argument, so the proposed solution is consistent. On the other hand, in probably all cases, this requires writing guard.get_mut().get_mut().

…tGuard.

The only way to use AsyncFdReadyMutGuard currently is through try_io,
since it is impossible to access the underlying object. In some
situations, it is desirable to use clear_ready and retain_ready directly
instead of try_io. Enable this by adding get_ref and get_mut methods.
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels May 23, 2021
@Darksonn
Copy link
Contributor

One option is to also add an get_inner_mut method?

@brain0
Copy link
Contributor Author

brain0 commented May 26, 2021

Yes. So, would you rather see four methods get_ref, get_mut, get_inner_ref and get_inner_mut, or only the latter two?

I personally doubt that accessing the AsyncFd<T> is useful in any way, I was confused when I saw that the closure passed to try_io takes AsyncFd<T> instead of T.

@Darksonn
Copy link
Contributor

We can add all four. Even if it isn't very useful right now, it might get new methods in the future where it is useful.

Comment on lines 483 to 491
/// Returns a shared reference to the inner [`AsyncFd`].
pub fn get_ref(&self) -> &AsyncFd<Inner> {
self.async_fd
}

/// Returns a shared reference to the backing object of the inner [`AsyncFd`].
pub fn get_inner(&self) -> &Inner {
self.get_ref().get_ref()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move the methods to the bottom of the impl block so they don't appear at the top of the documentation?

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants