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

add async_io for AsyncFd #5542

Merged
merged 5 commits into from Apr 16, 2023
Merged

Conversation

folkertdev
Copy link
Contributor

Motivation

In #5512, async_io was exposed on various socket types. However, for our work on ntpd-rs we would like this function to also be available on AsyncFd. We do some low-level epoll configuration there (specifically, listening for libc::EPOLLPRI on a separate file descriptor), so all we have is a file descriptor.

Solution

Expose the registration's async_io. The implementation is a single line. I believe that the implementation of Registration::async_io correctly clears the readiness flag, and that therefore use of AsyncFdReadyGuard is not needed here.

tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
@Darksonn Darksonn added A-tokio Area: The main tokio crate M-io Module: tokio/io labels Mar 15, 2023
@folkertdev
Copy link
Contributor Author

folkertdev commented Mar 15, 2023

there is one other idea I had. The implementation could instead be

pub async fn async_io<R>(
    &self,
    interest: Interest,
    mut f: impl FnMut(&T) -> io::Result<R>,
) -> io::Result<R> {
    self.registration
        .async_io(interest, || f(self.get_ref()))
        .await
}

which changes usage to

let output = async_fd
    .async_io(Interest::WRITABLE, |inner| inner.send(buf))
    .await;

this makes the type signature inconsistent with the one for other types (but the same is already true for the signature of try_io), but I think it is nice that async_fd does not need to be repeated.

I'm not sure how to weigh these concerns: should the api be consistent, or is it better to deviate slightly if it is nicer for a particular type?

@folkertdev folkertdev force-pushed the async-fd-async-io branch 4 times, most recently from e648d1e to 87c864f Compare March 21, 2023 10:15
@folkertdev
Copy link
Contributor Author

after thinking about it for a while, I think that second API I suggested is the better approach. It just makes using the API for this type more convenient, and the signature changes in a predictable way (try_io's signature follows the same pattern)

@Darksonn
Copy link
Contributor

This raises the question of whether we want an FnMut(&T) or FnMut(&mut T). The other AsyncFd methods have a variant for both.

@folkertdev
Copy link
Contributor Author

hmm, that would require two functions (so adding async_io_mut) maybe this is the reason the other async_ios don't provide the inner value? it works for try_io because there is a standard and a mutable guard.

I'm inclined to revert to the old api. thoughts?

@Darksonn
Copy link
Contributor

Darksonn commented Apr 8, 2023

Sorry for the delay. All of the other methods are duplicated, so I think it's fine for this one to be too.

@folkertdev
Copy link
Contributor Author

ok, added it. Two questions from my side

  • is this better than not giving the closure an argument (I think so)
  • is the documentation for async_io_mut sufficient this way?

tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
tokio/src/io/async_fd.rs Show resolved Hide resolved
tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
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.

You can fix the FreeBSD ci failure by merging in master.

tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
tokio/src/io/async_fd.rs Outdated Show resolved Hide resolved
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.

Just one more comment, then I think this is ready to be merged.

Comment on lines +512 to +518
/// The `async_io` method is a convenience utility that waits for the file
/// descriptor to become ready, and then executes the provided IO operation.
/// Since file descriptors may be marked ready spuriously, the closure will
/// be called repeatedly until it returns something other than a
/// [`WouldBlock`] error. This is done using the following loop:
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation of a function should start with a short, single line description of what it does.

You can use the same one as the one on async_io_mut.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

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.

Thanks. LGTM.

@Darksonn Darksonn merged commit 6037fae into tokio-rs:master Apr 16, 2023
52 checks passed
crapStone pushed a commit to Calciumdibromid/CaBr2 that referenced this pull request Apr 29, 2023
This PR contains the following updates:

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

---

### Release Notes

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

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

[Compare Source](tokio-rs/tokio@tokio-1.27.0...tokio-1.28.0)

##### 1.28.0 (April 25th, 2023)

##### Added

-   io: add `AsyncFd::async_io` ([#&#8203;5542])
-   io: impl BufMut for ReadBuf ([#&#8203;5590])
-   net: add `recv_buf` for `UdpSocket` and `UnixDatagram` ([#&#8203;5583])
-   sync: add `OwnedSemaphorePermit::semaphore` ([#&#8203;5618])
-   sync: add `same_channel` to broadcast channel ([#&#8203;5607])
-   sync: add `watch::Receiver::wait_for` ([#&#8203;5611])
-   task: add `JoinSet::spawn_blocking` and `JoinSet::spawn_blocking_on` ([#&#8203;5612])

##### Changed

-   deps: update windows-sys to 0.48 ([#&#8203;5591])
-   io: make `read_to_end` not grow unnecessarily ([#&#8203;5610])
-   macros: make entrypoints more efficient ([#&#8203;5621])
-   sync: improve Debug impl for `RwLock` ([#&#8203;5647])
-   sync: reduce contention in `Notify` ([#&#8203;5503])

##### Fixed

-   net: support `get_peer_cred` on AIX ([#&#8203;5065])
-   sync: avoid deadlocks in `broadcast` with custom wakers ([#&#8203;5578])

##### Documented

-   sync: fix typo in `Semaphore::MAX_PERMITS` ([#&#8203;5645])
-   sync: fix typo in `tokio::sync::watch::Sender` docs ([#&#8203;5587])

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

</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:eyJjcmVhdGVkSW5WZXIiOiIzNS42My4xIiwidXBkYXRlZEluVmVyIjoiMzUuNjQuMCJ9-->

Co-authored-by: cabr2-bot <cabr2.help@gmail.com>
Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1875
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants