Add documentation for SyncIoBridge with examples and alternatives #6815

Nathy-bajo · 2024-09-03T10:50:05Z

Motivation

The SyncIoBridge documentation lacked clarity on its usage, potentially leading to inefficient use and suboptimal performance. Users needed better guidance on when to use it and how to handle common use cases effectively.

Solution

This update enhances the SyncIoBridge documentation by:

•	Explaining its purpose and correct usage.
•	Providing practical examples and scenarios for effective use.
•	Highlighting alternatives to avoid blocking the async runtime.
•	Adding cross-references to related documentation.

These improvements aim to help users use SyncIoBridge more effectively and avoid common pitfalls.

Rendered

tglane · 2024-09-03T15:40:49Z

tokio-util/src/io/sync_bridge.rs

+///    let mut file = File::open("output.txt")?;
+///    std::io::copy(&mut sync_reader, &mut file)?;
+///    Ok::<_, std::io::Error>(())
+/// }).await??;


To please rustfmt in the CI I think you need to move the .await??; into its own line.

Suggested change

/// }).await??;

/// })

/// .await??;

I have seen my mistake.
Thank you!

tglane · 2024-09-03T17:58:19Z

tokio-util/src/io/sync_bridge.rs

+///
+/// ```rust, no_run
+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;


For async/await to work in the docs you actually also need a runtime since the doc test will be run by a synchronous main function by default.

See this from tokio as an example:

/// ```rust,no_run /// use std::error::Error; /// /// #[tokio::main] /// async fn main() -> Result<(), Box<dyn Error>> { /// let tokio_listener = tokio::net::TcpListener::bind("127.0.0.1:0").await?; /// let std_listener = tokio_listener.into_std()?; /// std_listener.set_nonblocking(false)?; /// Ok(()) /// } /// ```

hawkw

Thanks for the PR!

I think that most of the examples added here don't actually compile. Before we can merge this, we need to make them compile. I left some suggestions on how to do that.

hawkw · 2024-09-03T18:40:45Z

tokio-util/src/io/sync_bridge.rs

+///
+/// ## Example 1: Hashing Data
+///
+/// When hashing data, using `SyncIoBridge` can lead to suboptimal performance and might not fully leverage the async capabilities of the system.


It would be nice if this explained why using SyncIoBridge "can lead to suboptimal performance and might not fully leverage the async capabilities of the system"; I think just saying "you might have bad performance" is not as helpful as explaining the specific details, so that the user can know what issues they are avoiding and why they matter?

hawkw · 2024-09-03T18:43:33Z

tokio-util/src/io/sync_bridge.rs

+/// ```rust, no_run
+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;
+/// let hash = blake3::hash(&data);
+/// ```
+///
+/// Or, for more complex cases:
+///
+/// ```rust, no_run
+/// let mut data = vec![0; 64 * 1024];
+/// loop {
+///     let len = reader.read(&mut data).await?;
+///     if len == 0 { break; }
+///     hasher.update(&data[..len]);
+/// }
+/// let hash = hasher.finalize();
+/// ```


It would be nice to have a few comments in these examples explaining what they're doing...

hawkw · 2024-09-03T18:44:52Z

tokio-util/src/io/sync_bridge.rs

+/// ## Example 2: Compressing Data
+///
+/// When compressing data, avoid using `SyncIoBridge`` with non-async compression libraries, as it may lead to inefficient and blocking code.
+/// Instead, use `async-compression`, which is designed to work with asynchronous data streams.


I'd prefer not to recommend a specific library in the tokio documentation, because it might not work for all use-cases. I might say

Suggested change

/// Instead, use `async-compression`, which is designed to work with asynchronous data streams.

/// Instead, use an async compression library, such as the `async-compression` crate, which is designed to work with asynchronous data streams.

Also, if we are going to recommend this crate specifically, we should probably link to its documentation.

hawkw · 2024-09-03T18:46:21Z

tokio-util/src/io/sync_bridge.rs

+/// ## Example 3: Parsing `JSON`
+///
+/// When parsing `JSON` data, avoid using `SyncIoBridge` with `serde_json::from_reader` as it may cause blocking operations.
+/// Instead, read the data into a `Vec<u8>` and parse it using `serde_json::from_slice`.
+///
+/// ```rust, no_run
+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;
+/// let value: MyStruct = serde_json::from_slice(&data)?;
+/// ```


This isn't really specific to JSON; this advice applies equally to any other serialization format. What about something like:

When parsing serialization formats such as JSON, avoid using SyncIoBridge with functions that deserialize data from a type implementing std::io::Read, such as serde_json::from_reader.

hawkw · 2024-09-03T18:48:35Z

tokio-util/src/io/sync_bridge.rs

I think a number of the examples in this file won't actually compile. While it's fine to not run the examples using no_run, they should, at least, compile --- the test suite will fail if examples contain code that doesn't compile. In many cases, the examples reference names that aren't actually defined, such as &mut reader and &mut writer when there is no value named reader or writer in the example. We should add hidden definitions of these values to the examples to make sure they compile.

hawkw · 2024-09-03T18:50:48Z

tokio-util/src/io/sync_bridge.rs

+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;


Suggested change

/// let mut data = Vec::new();

/// reader.read_to_end(&mut data).await?;

/// use tokio::io::AsyncReadExt;

///

/// # let mut reader = tokio::io::empty();

/// let mut data = Vec::new();

/// reader.read_to_end(&mut data).await?;

hawkw · 2024-09-03T18:51:35Z

tokio-util/src/io/sync_bridge.rs

+/// ```rust, no_run
+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;
+/// let hash = blake3::hash(&data);


I don't think this will compile, because there is no blake3 in scope. We can fix this by adding a hidden dummy version, like:

Suggested change

/// let hash = blake3::hash(&data);

/// let hash = blake3::hash(&data);

/// # mod blake3 { pub fn hash(_: &[u8]) {} }

hawkw · 2024-09-03T18:51:59Z

tokio-util/src/io/sync_bridge.rs

+/// ```rust, no_run
+/// let mut data = vec![0; 64 * 1024];
+/// loop {
+///     let len = reader.read(&mut data).await?;
+///     if len == 0 { break; }
+///     hasher.update(&data[..len]);
+/// }
+/// let hash = hasher.finalize();


this won't compile because hasher and reader are not defined in this scope.

hawkw · 2024-09-03T18:52:30Z

tokio-util/src/io/sync_bridge.rs

+/// use async_compression::tokio::write::GzipEncoder;
+///
+/// let mut encoder = GzipEncoder::new(writer);
+/// tokio::io::copy(&mut reader, &mut encoder).await?;


this won't compile, because async_compression is not a dependency, and reader and writer are not defined.

hawkw · 2024-09-03T18:52:54Z

tokio-util/src/io/sync_bridge.rs

+/// let mut data = Vec::new();
+/// reader.read_to_end(&mut data).await?;
+/// let value: MyStruct = serde_json::from_slice(&data)?;


this won't compile because reader is undefined, and serde_json is not a dependency of the example.

Nathy-bajo · 2024-09-04T09:58:21Z

Hi @hawkw.
Thank you for the suggestions! I have updated the codes.

Nathy-bajo · 2024-09-06T10:36:47Z

Is there anything else I need to add? @hawkw @Darksonn @tglane

Darksonn · 2024-09-10T12:12:06Z

tokio-util/src/io/sync_bridge.rs

+/// When hashing data, using `SyncIoBridge` can lead to suboptimal performance and might not fully leverage the async capabilities of the system.
+///
+/// ### Why It Matters:
+/// `SyncIoBridge` allows you to use synchronous I/O operations in an asynchronous context by blocking the current thread. However, this can be inefficient because:
+///  Blocking: The use of `SyncIoBridge` may block a valuable async runtime thread, which could otherwise be used to handle more tasks concurrently.
+///  Thread Pool Saturation: If many threads are blocked using `SyncIoBridge`, it can exhaust the async runtime's thread pool, leading to increased latency and reduced throughput.
+///  Lack of Parallelism: By blocking on synchronous operations, you may miss out on the benefits of running tasks concurrently, especially in I/O-bound operations where async tasks could be interleaved.
+///
+/// Instead, consider reading the data into memory and then hashing it, or processing the data in chunks.


I think there are several opportunities to improve the wording here.

The explanation for why you should try to avoid SyncIoBridge should go before ## Example 1 and after # Alternatives. It applies to all examples, not just the first one.

Each example should start by mentioning that the use of SyncIoBridge is unnecessary.

For each example, there should be a short explanation of what the example does. See below for explanations for the hashing example. Just saying "or for more complex cases" is very generic, and doesn't actually explain when a case is complex.

Your text should be formatted according to markdown. You don't put newlines at the end of sentences like that. Instead, wrap the line after 80 characters.

/// ## Hashing data /// /// It is common to use `SyncIoBridge` for hashing data, but this is unnecessary /// in most cases. /// /// There are two strategies for avoiding `SyncIoBridge` when hashing data. When /// the data fits into memory, the easiest is to read the data into a `Vec<u8>` /// and hash it: /// /// [ first example ] /// /// When the data doesn't fit into memory, the hashing library will usually /// provide a hasher that you can repeatedly call `update` on to hash the data /// one chunk at the time. For example: /// /// [second example]

Darksonn · 2024-09-10T12:18:43Z

tokio-util/src/io/sync_bridge.rs

+/// #[tokio::main]
+/// async fn main() -> Result<(), Box<dyn std::error::Error>> {
+///     let mut reader = tokio::io::empty();


Instead of using tokio::io::empty(), I think it makes more sense to define a function that takes any AsyncRead.

async fn hash_contents(reader: impl AsyncRead) -> std::io::Result<()> {

Also, please avoid using Box<dyn Error> as that type is not Send. Instead, you can use std::io::Error for your examples.

Nathy-bajo · 2024-09-15T22:46:10Z

Hi @Darksonn
I have updated the PR. Is there anything else needed?

Nathy-bajo mentioned this pull request Sep 3, 2024

Recommend alternatives to SyncIoBridge #6795

Open

6 tasks

Darksonn added T-docs Topic: documentation A-tokio-util Area: The tokio-util crate M-io Module: tokio/io labels Sep 3, 2024

tglane reviewed Sep 3, 2024

View reviewed changes

Nathy-bajo force-pushed the sync-io-bridge-doc branch from 6673e3e to 30d38d5 Compare September 3, 2024 17:09

tglane reviewed Sep 3, 2024

View reviewed changes

hawkw requested changes Sep 3, 2024

View reviewed changes

Nathy-bajo added 3 commits September 4, 2024 11:05

Add documentation for SyncIoBridge with examples and alternatives

699fd78

Add documentation for SyncIoBridge with examples and alternatives

de14fef

Add documentation for SyncIoBridge with examples and alternatives

6bdda28

Nathy-bajo force-pushed the sync-io-bridge-doc branch from cd8edbc to 6bdda28 Compare September 4, 2024 10:07

Nathy-bajo added 2 commits September 4, 2024 11:12

code formatting

47534ed

cargo spell check error fix

d9dc4bf

Darksonn reviewed Sep 10, 2024

View reviewed changes

Nathy-bajo added 5 commits September 11, 2024 23:03

Add documentation for SyncIoBridge with examples and alternatives

1830b14

Add documentation for SyncIoBridge with examples and alternatives

234f9f3

Fix build error for i686-unknown-linux-gnu target in Tokio tests

9e02ebe

cargo fmt error fix

4f8c220

cargo spell check error fix

af07465

Darksonn requested a review from hawkw September 16, 2024 20:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add documentation for SyncIoBridge with examples and alternatives #6815

Add documentation for SyncIoBridge with examples and alternatives #6815

Nathy-bajo commented Sep 3, 2024 •

edited by Darksonn

Loading

tglane Sep 3, 2024

Nathy-bajo Sep 3, 2024

tglane Sep 3, 2024

hawkw left a comment

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

hawkw Sep 3, 2024

Nathy-bajo commented Sep 4, 2024 •

edited

Loading

Nathy-bajo commented Sep 6, 2024

Darksonn Sep 10, 2024

Darksonn Sep 10, 2024

Nathy-bajo commented Sep 15, 2024

	/// Instead, use `async-compression`, which is designed to work with asynchronous data streams.
	/// Instead, use an async compression library, such as the `async-compression` crate, which is designed to work with asynchronous data streams.

		/// let mut data = Vec::new();
		/// reader.read_to_end(&mut data).await?;

	/// let hash = blake3::hash(&data);
	/// let hash = blake3::hash(&data);
	/// # mod blake3 { pub fn hash(_: &[u8]) {} }

Add documentation for SyncIoBridge with examples and alternatives #6815

Are you sure you want to change the base?

Add documentation for SyncIoBridge with examples and alternatives #6815

Conversation

Nathy-bajo commented Sep 3, 2024 • edited by Darksonn Loading

Motivation

Solution

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hawkw left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nathy-bajo commented Sep 4, 2024 • edited Loading

Nathy-bajo commented Sep 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nathy-bajo commented Sep 15, 2024

Nathy-bajo commented Sep 3, 2024 •

edited by Darksonn

Loading

Nathy-bajo commented Sep 4, 2024 •

edited

Loading