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.

Sign up for GitHub

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

Jump to bottom

Fix futures_io::AsyncSeek implementaion for Compat #5783

Merged
merged 7 commits into from
Jun 25, 2023
Merged

Fix futures_io::AsyncSeek implementaion for Compat #5783

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
51c38cb
Return Poll::Pending until seek completes
dhruv9vats Jun 10, 2023
076afe9
Use ready! and add test
dhruv9vats Jun 13, 2023
fd3f9d5
Merge remote-tracking branch 'upstream/master' into tokio-util-compat…
dhruv9vats Jun 13, 2023
29a63dd
Attempt to fix CI
dhruv9vats Jun 13, 2023
61f50a1
Remove incosistent unwarp use
dhruv9vats Jun 13, 2023
8f4d239
Revert unintended Cargo.toml format
dhruv9vats Jun 25, 2023
b707b0a
Merge remote-tracking branch 'upstream/master' into tokio-util-compat…
dhruv9vats Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions tokio-util/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ async-stream = "0.3.0"
futures = "0.3.0"
futures-test = "0.3.5"
parking_lot = "0.12.0"
tempfile = "3.1.0"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding a dependency to add a test feels overkill.
But the thing is, the test only fails for a tokio::fs::file::File.
Using something like tokio::io::util::buf_writer::BufWriter or std::io::cursor::Cursor seems to be working fine on latest upstream/master:

#[tokio::test]
async fn compat_seek() -> futures_util::io::Result<()> {
    let mut cursor = Cursor::new(Vec::new()).compat_write();

    cursor.write_all(&[0, 1, 2, 3, 4, 5]).await?;
    cursor.write_all(&[6, 7]).await?;

    assert_eq!(cursor.stream_position().await?, 8);

    // Modify elements at position 2.
    assert_eq!(cursor.seek(SeekFrom::Start(2)).await?, 2);
    cursor.write_all(&[8, 9]).await?;

    cursor.flush().await?;

    // Verify we still have 8 elements
    assert_eq!(cursor.seek(SeekFrom::End(0)).await?, 8);
    // Seek back to the start to read and verify contents.
    cursor.seek(SeekFrom::Start(0)).await?;

    let mut buf = Vec::new();
    let num_bytes = cursor.read_to_end(&mut buf).await?;
    assert_eq!(&buf[..num_bytes], &[0, 1, 8, 9, 4, 5, 6, 7]);

    Ok(())
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Darksonn, is there some alternative way to test this? And would it more sense to skip adding one if there is none?

Copy link
Contributor

Choose a reason for hiding this comment

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

This dependency already exists in the main Tokio crate, so adding it here doesn't actually add a new dependency.


[package.metadata.docs.rs]
all-features = true
Expand Down
2 changes: 2 additions & 0 deletions tokio-util/src/compat.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,8 @@ impl<T: tokio::io::AsyncSeek> futures_io::AsyncSeek for Compat<T> {
pos: io::SeekFrom,
) -> Poll<io::Result<u64>> {
if self.seek_pos != Some(pos) {
// Ensure previous seeks have finished before starting a new one
ready!(self.as_mut().project().inner.poll_complete(cx))?;
self.as_mut().project().inner.start_seek(pos)?;
*self.as_mut().project().seek_pos = Some(pos);
}
Expand Down
43 changes: 43 additions & 0 deletions tokio-util/tests/compat.rs
View file
Open in desktop
dhruv9vats marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
#![cfg(all(feature = "compat"))]
#![cfg(not(target_os = "wasi"))] // WASI does not support all fs operations
#![warn(rust_2018_idioms)]

use futures_io::SeekFrom;
use futures_util::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt};
use tempfile::NamedTempFile;
use tokio::fs::OpenOptions;
use tokio_util::compat::TokioAsyncWriteCompatExt;

#[tokio::test]
async fn compat_file_seek() -> futures_util::io::Result<()> {
let temp_file = NamedTempFile::new()?;
let mut file = OpenOptions::new()
.read(true)
.write(true)
.create(true)
.open(temp_file)
.await?
.compat_write();

file.write_all(&[0, 1, 2, 3, 4, 5]).await?;
file.write_all(&[6, 7]).await?;

assert_eq!(file.stream_position().await?, 8);

// Modify elements at position 2.
assert_eq!(file.seek(SeekFrom::Start(2)).await?, 2);
file.write_all(&[8, 9]).await?;

file.flush().await?;

// Verify we still have 8 elements.
assert_eq!(file.seek(SeekFrom::End(0)).await?, 8);
// Seek back to the start of the file to read and verify contents.
file.seek(SeekFrom::Start(0)).await?;

let mut buf = Vec::new();
let num_bytes = file.read_to_end(&mut buf).await?;
assert_eq!(&buf[..num_bytes], &[0, 1, 8, 9, 4, 5, 6, 7]);

Ok(())
}