util/io: Add `SyncIoBridge` #4146

cgwalters · 2021-10-01T21:28:57Z

Motivation

I'm doing quite a bit of stuff inside spawn_blocking because
I need to use some large synchronous APIs.

I found it not entirely obvious how to "bridge" the world of
async I/O with sync I/O.

Since we have a handy "tokio-util" module with an "io" feature,
these small helpers seem like a good fit hopefully!

Solution

Extracted these from my code in https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/async_util.rs

Darksonn

Why not create a single type with both impls? Then it is easier to bridge a type with both traits.

tokio-util/src/io/sync_bridge.rs

cgwalters · 2021-10-03T13:51:56Z

Why not create a single type with both impls? Then it is easier to bridge a type with both traits.

Good suggestion, done!

tokio-util/src/io/sync_bridge.rs

hawkw · 2021-10-03T16:58:16Z

tokio-util/src/io/sync_bridge.rs

+/// Use a [`tokio::io::AsyncRead`] synchronously as a [`std::io::Read`] or
+/// a [`tokio::io::AsyncWrite`] as a [`std::io::Write`].
+#[derive(Debug)]
+pub struct SyncIOBridge<T> {


Nit: in general, idiomatic Rust naming treats acronyms (like "IO") as one "word", rather than capitalizing every letter: https://doc.rust-lang.org/1.0.0/style/style/naming/README.html#general-conventions-[rfc-#430]

So, this should be

Suggested change

pub struct SyncIOBridge<T> {

pub struct SyncIoBridge<T> {

also, take it or leave it, but we could probably just drop the Io from the name of this, since it's already in the io module? i'm fine either way though.

Yeah, I was uncertain about the Io. What makes me lean towards leaving it in is that there's also e.g. the whole tokio::sync module which is about synchronization as opposed to "synchronous". I don't expect this type to be frequently used, and it's probably worth being a bit more verbose to highlight the conversion? But if someone feels strongly I'm obviously fine changing!

Maybe we could use the term "blocking", e.g. BlockingIoWrapper?

Or actually, there's already some precendent in tokio for from_std so we could call it StdIoWrapper (or StdIoBridge)?

I think Blocking or Sync is better than Std here. I don't particularly mind including Io in the name.

OK...keeping this as SyncIoBridge then just from inertia basically. But happy to change if someone else feels strongly.

tokio-util/src/io/sync_bridge.rs

tokio-util/src/io/mod.rs

tokio-util/src/io/sync_bridge.rs

I'm working on some code which heavily uses `spawn_blocking` to run synchronous code, and it took me a while to find and understand the relevant APIs and patterns here. Let's link to the channel blocking APIs, and provide a small example. I plan to mention tokio-rs#4146 here too once that merges.

tokio-util/src/io/sync_bridge.rs

Darksonn · 2021-10-04T17:28:27Z

When you make changes to this PR, please avoid force pushing as it makes it difficult for me to see what changed since last time I looked. Do not worry about there being lots of commits — they will be squashed when we merge the PR.

cgwalters · 2021-10-04T18:16:35Z

When you make changes to this PR, please avoid force pushing as it makes it difficult for me to see what changed since last time I looked.

OK, though I think I had some local changes before I saw this, so there is a final force here, but subsequent update commits.
(That said, github does have a minimal interdiff view).

tokio-util/src/io/sync_bridge.rs

Darksonn · 2021-10-05T13:52:51Z

The CI failure is due to you being very far behind the master branch.

I'm doing quite a bit of stuff inside `spawn_blocking` because I need to use some large synchronous APIs. I found it not entirely obvious how to "bridge" the world of async I/O with sync I/O. Since we have a handy "tokio-util" module with an "io" feature, these small helpers seem like a good fit hopefully! Extracted these from my code in https://github.com/ostreedev/ostree-rs-ext/blob/main/lib/src/async_util.rs

cgwalters · 2021-10-05T14:02:18Z

Rebased 🏄

cgwalters · 2021-10-20T00:53:32Z

(This is really the first time I've tried to debug issues in a multi-crate git repository with crosscutting features)

OK, I was misreading the CI before, at least one failure reproduces here with: cargo +nightly check -Z avoid-dev-deps --no-default-features --features full.

I initially tried sprinkling cfg_io! etc. in different places, but that didn't work. It took me a little while to realize I was getting confused between the tokio io-util feature and the tokio-util io feature. And here, we really need to require the tokio feature (right?).

So this fixes it for me:

diff --git a/tokio-util/Cargo.toml b/tokio-util/Cargo.toml
index 8c0481ec..f81734b3 100644
--- a/tokio-util/Cargo.toml
+++ b/tokio-util/Cargo.toml
@@ -35,7 +35,7 @@ rt = ["tokio/rt"]
 __docs_rs = ["futures-util"]
 
 [dependencies]
-tokio = { version = "1.0.0", path = "../tokio", features = ["sync"] }
+tokio = { version = "1.0.0", path = "../tokio", features = ["sync", "io-util"] }
 
 bytes = "1.0.0"
 futures-core = "0.3.0"

but would that be OK? Or would it be necessary to add a new tokio-util feature that in turn enables the io-util tokio feature?

Darksonn · 2021-10-20T07:00:49Z

We could add an io-util feature to tokio-util that enables the io-util feature of Tokio.

Necessary for the sync bridge work. I don't know whether this is OK.

cgwalters · 2021-10-20T16:29:26Z

Now that I looked it is easier, we just need to have the tokio-util/io feature depend on tokio/io-util (the terms here are confusing!)

We could add an io-util feature to tokio-util that enables the io-util feature of Tokio.

That said, happy to do that instead if the tokio/io-util feature is potentially problematic for users of tokio-util/io. I honestly do not have a strong sense of which tokio users are selecting which features. The depchain of tokio/io-util seems like it's just ['memchr', 'bytes'] and tokio-util already hard depends on bytes, so after this change, requesting tokio-util/io now just pulls in memchr or so which seems quite small.

tokio-util/src/io/sync_bridge.rs

cgwalters · 2021-10-21T13:35:30Z

Ahh. Right, we also need rt. Hmm, I think that's going to force us to go to the separate tokio-util/io-util feature route?

Move sync-bridge to it.

tokio-util/Cargo.toml

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Darksonn · 2021-10-21T21:34:52Z

If CI keeps failing for unrelated reasons, you may want to merge in master to your PR.

Darksonn · 2021-10-22T06:57:29Z

I figured out what the problem is. #4189

cgwalters · 2021-10-22T13:03:22Z

I figured out what the problem is. #4189

Ah nice, thanks.

Darksonn

This seems good!

Darksonn · 2021-10-22T16:46:57Z

If you want, you could amend the Bridging with sync code topics page with a section on this utility.

cgwalters · 2021-10-22T21:04:42Z

If you want, you could amend the Bridging with sync code topics page with a section on this utility.

Oh wow, I somehow completely missed that before. Should I update #4150 to link to that too?

Darksonn · 2021-10-23T09:44:09Z

Sure.

We can only land this after a new tokio is released with tokio-rs/tokio#4146

cgwalters · 2021-10-26T15:44:40Z

xref #4150 (comment)

We can only land this after a new tokio is released with tokio-rs/tokio#4146

cgwalters · 2022-07-22T13:51:56Z

From time to time I like to explicitly say "thank you" in a Github comment. A lot of time Github conversations can be about bug reports, people upset about regressions, contentious debates, etc.

This was my first real PR to this repository and you all took the time to work with me, patiently found issues and help me learn new things, and it was overall a great experience. It definitely led to me feeling positive about contributing to tokio again!

So: thank you! 🙏

Darksonn · 2022-07-22T13:59:13Z

That's great! I'm happy that you found it to be a good experience.

Darksonn added A-tokio-util Area: The tokio-util crate M-io Module: tokio/io labels Oct 2, 2021

Darksonn reviewed Oct 2, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

cgwalters force-pushed the sync-io-bridge branch from 5040268 to e64500f Compare October 3, 2021 13:51

cgwalters force-pushed the sync-io-bridge branch 2 times, most recently from e3bef63 to 913b94e Compare October 3, 2021 13:53

hawkw reviewed Oct 3, 2021

View reviewed changes

cgwalters force-pushed the sync-io-bridge branch 2 times, most recently from 15d2f0a to e729ec2 Compare October 3, 2021 18:23

Darksonn reviewed Oct 4, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

Darksonn reviewed Oct 4, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

cgwalters mentioned this pull request Oct 4, 2021

task: Add more tips + links to spawn_blocking docs #4150

Merged

cgwalters force-pushed the sync-io-bridge branch 2 times, most recently from cb4f217 to c5f145d Compare October 4, 2021 13:38

cgwalters mentioned this pull request Oct 4, 2021

lib/tar: Pre-filter tar archive in Rust ostreedev/ostree-rs-ext#112

Merged

Darksonn reviewed Oct 4, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

tokio-util/src/io/sync_bridge.rs Show resolved Hide resolved

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

cgwalters force-pushed the sync-io-bridge branch from c5f145d to e82099b Compare October 4, 2021 18:15

Darksonn reviewed Oct 5, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Show resolved Hide resolved

cgwalters added 5 commits October 5, 2021 10:02

fix bounds

ca04164

update docs

0b35775

forward more read methods

2d5c9c8

Mention !Unpin types

59a9566

Have io depend on tokio/io-util for SyncIoBridge

541011b

Necessary for the sync bridge work. I don't know whether this is OK.

Darksonn reviewed Oct 21, 2021

View reviewed changes

tokio-util/src/io/sync_bridge.rs Outdated Show resolved Hide resolved

Remove unncessary build constraint

2d8b60c

cgwalters added 2 commits October 21, 2021 09:47

util: Create a new io-util feature that depends on rt and io

e7bab76

Move sync-bridge to it.

util: We only need io-util macro if we have io

0a91e6d

Darksonn reviewed Oct 21, 2021

View reviewed changes

tokio-util/Cargo.toml Outdated Show resolved Hide resolved

Update tokio-util/Cargo.toml

0584c03

Co-authored-by: Alice Ryhl <aliceryhl@google.com>

Merge branch 'master' into sync-io-bridge

b8956f3

Merge remote-tracking branch 'origin/master' into sync-io-bridge

2725071

Darksonn approved these changes Oct 22, 2021

View reviewed changes

Darksonn merged commit 2734fa9 into tokio-rs:master Oct 22, 2021

cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request Oct 26, 2021

Use tokio SyncIoBridge

a3662b3

We can only land this after a new tokio is released with tokio-rs/tokio#4146

cgwalters mentioned this pull request Oct 26, 2021

Use tokio SyncIoBridge ostreedev/ostree-rs-ext#128

Merged

Darksonn mentioned this pull request Oct 26, 2021

chore: prepare tokio-util 0.6.9 #4199

Merged

oliver-giersch pushed a commit to oliver-giersch/tokio that referenced this pull request Oct 28, 2021

util/io: add SyncIoBridge (tokio-rs#4146)

d2d2341

cgwalters added a commit to cgwalters/ostree-rs-ext that referenced this pull request Oct 30, 2021

Use tokio SyncIoBridge

1ed8e29

We can only land this after a new tokio is released with tokio-rs/tokio#4146

imlk0 mentioned this pull request Aug 23, 2022

Add a way to shutdown SyncIoBridge #4932

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util/io: Add `SyncIoBridge` #4146

util/io: Add `SyncIoBridge` #4146

cgwalters commented Oct 1, 2021

Darksonn left a comment

cgwalters commented Oct 3, 2021

hawkw Oct 3, 2021

hawkw Oct 3, 2021

cgwalters Oct 3, 2021

Darksonn Oct 4, 2021

cgwalters Oct 4, 2021

Darksonn commented Oct 4, 2021

cgwalters commented Oct 4, 2021

Darksonn commented Oct 5, 2021 •

edited

Loading

cgwalters commented Oct 5, 2021

cgwalters commented Oct 20, 2021 •

edited

Loading

Darksonn commented Oct 20, 2021

cgwalters commented Oct 20, 2021

cgwalters commented Oct 21, 2021

Darksonn commented Oct 21, 2021

Darksonn commented Oct 22, 2021

cgwalters commented Oct 22, 2021

Darksonn left a comment

Darksonn commented Oct 22, 2021

cgwalters commented Oct 22, 2021

Darksonn commented Oct 23, 2021

cgwalters commented Oct 26, 2021

cgwalters commented Jul 22, 2022

Darksonn commented Jul 22, 2022

util/io: Add SyncIoBridge #4146

util/io: Add SyncIoBridge #4146

Conversation

cgwalters commented Oct 1, 2021

Motivation

Solution

Darksonn left a comment

Choose a reason for hiding this comment

cgwalters commented Oct 3, 2021

hawkw Oct 3, 2021

Choose a reason for hiding this comment

hawkw Oct 3, 2021

Choose a reason for hiding this comment

cgwalters Oct 3, 2021

Choose a reason for hiding this comment

Darksonn Oct 4, 2021

Choose a reason for hiding this comment

cgwalters Oct 4, 2021

Choose a reason for hiding this comment

Darksonn commented Oct 4, 2021

cgwalters commented Oct 4, 2021

Darksonn commented Oct 5, 2021 • edited Loading

cgwalters commented Oct 5, 2021

cgwalters commented Oct 20, 2021 • edited Loading

Darksonn commented Oct 20, 2021

cgwalters commented Oct 20, 2021

cgwalters commented Oct 21, 2021

Darksonn commented Oct 21, 2021

Darksonn commented Oct 22, 2021

cgwalters commented Oct 22, 2021

Darksonn left a comment

Choose a reason for hiding this comment

Darksonn commented Oct 22, 2021

cgwalters commented Oct 22, 2021

Darksonn commented Oct 23, 2021

cgwalters commented Oct 26, 2021

cgwalters commented Jul 22, 2022

Darksonn commented Jul 22, 2022

util/io: Add `SyncIoBridge` #4146

util/io: Add `SyncIoBridge` #4146

Darksonn commented Oct 5, 2021 •

edited

Loading

cgwalters commented Oct 20, 2021 •

edited

Loading