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

std::io::Write::write_vectored() to io::stdout() does not produce all expected lines. #68041

Closed
rustyconover opened this issue Jan 9, 2020 · 17 comments
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@rustyconover
Copy link

rustyconover commented Jan 9, 2020

When trying to write an array of 8192 IoSlices consisting of "1" followed by "\n" only the first character is written on Mac OS X.

use std::io;
use std::io::prelude::*;
use std::io::IoSlice;

fn main() {
    let mut write_array: Vec<IoSlice> = Vec::new();
    for _ in 1..1024 * 4 {
        write_array.push(IoSlice::new(b"1"));
        write_array.push(IoSlice::new(b"\n"));
    }
    io::stdout().write_vectored(&write_array).unwrap();
    io::stdout().flush().unwrap();
}

Output:

1

Just one byte is written:

./target/debug/vector-bug | wc 
       0       1       1

I expected 4096 pairs of "1\n" to be written.

Meta

$ rustc --version --verbose
rustc 1.40.0 (73528e339 2019-12-16)
binary: rustc
commit-hash: 73528e339aae0f17a15ffa49a8ac608f50c6cf14
commit-date: 2019-12-16
host: x86_64-apple-darwin
release: 1.40.0
LLVM version: 9.0
@jonas-schievink
Copy link
Contributor

On nightly this panics with an InvalidInput I/O error instead (at least on the playground).

@jonas-schievink jonas-schievink added C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Jan 9, 2020
@tmiasko
Copy link
Contributor

tmiasko commented Jan 9, 2020

The write_vectored was implemented for LineWriter in 2fee28e, which changed behaviour here.
Now all buffers will be written at once and most likely trigger OS specific limit as described in #68042.

@sfackler
Copy link
Member

sfackler commented Jan 9, 2020

Like write, write_vectored does not necessarily write the entire set of data out. You need to check the value it returns.

@sfackler sfackler closed this as completed Jan 9, 2020
@jonas-schievink
Copy link
Contributor

This behavior does not match the documentation:

Data is copied from each buffer in order, with the final buffer read from possibly being only partially consumed.

@sfackler
Copy link
Member

sfackler commented Jan 9, 2020

"final buffer" means the final buffer that was read from, not the last buffer in the input slice. A concrete example of short writes beyond the OS doing that is the default implementation:

The default implementation calls write with either the first nonempty buffer provided, or an empty one if none exists.

@rustyconover
Copy link
Author

Would there be opposition to adding write_all_vectored() rather than having everybody implement the same logic of retrying until all data is written?

@sfackler
Copy link
Member

sfackler commented Jan 9, 2020

I think that would definitely make sense, though the implementation would be pretty tricky. Even more reason that it should be done once though I guess!

@rustyconover
Copy link
Author

rustyconover commented Jan 9, 2020 via email

@rustyconover
Copy link
Author

It seems others are working on it in #62726, but are blocked since they can't advance the pointer inside of the pending or partially written IoSlice.

@rustyconover
Copy link
Author

Here is an example write_all_vectored that works with nightly:

fn write_all_vectored(file: &mut File, mut bufs: &mut [IoSlice<'_>]) -> std::io::Result<()> {
    while bufs.len() > 0 {
        match file.write_vectored(bufs) {
            Ok(0) => {
                return Err(std::io::Error::new(
                    std::io::ErrorKind::WriteZero,
                    "failed to write whole buffer",
                ));
            }
            Ok(n) => bufs = IoSlice::advance(bufs, n),
            Err(ref e) if e.kind() == std::io::ErrorKind::Interrupted => {}
            Err(e) => return Err(e),
        }
    }
    Ok(())
}

@sfackler
Copy link
Member

sfackler commented Jan 9, 2020

That modifies the input slice array, which may or may not be a thing we'd want in an implementation in Read itself.

@rustyconover
Copy link
Author

You mean write?

@sfackler
Copy link
Member

sfackler commented Jan 9, 2020

Er, yeah.

@rustyconover
Copy link
Author

rustyconover commented Jan 9, 2020

How do you think it could be done by not modifying the array? write_vectored() could write a partial buffer necessitating the need to change an underlying IoSlice.

@sfackler
Copy link
Member

It could do a non-vectored write to finish off half written buffers, or very carefully swap out the partial buffer with an adjusted one and then fix it up after.

@rustyconover
Copy link
Author

I think performing a non-vectored write or many of them would not be what the person writing the code intends.

For example if I have a million element array of &str and I want to write all of them using but also output a newline after each &str I'd use the example code I wrote when opening the issue.

If after the first writev() Rust switches do doing write() for each buffer that will lead a whole lot more system calls and worse performance. If the array of IoSlices isn't mutable it will need to be copied to perform more calls to write_vectored() when the entire array cannot be written at once. I think that it may be correct that write_all_vectored() does need the mutability and consumes the array of IoSlices passed to it.

@sfackler
Copy link
Member

It wouldn't be doing a write for each buffer after the first writev, it would at worst be alternating between the two.

bors added a commit to rust-lang/rust-clippy that referenced this issue Jan 24, 2020
Lint vectored IO in unused_io_amount lint

`read_vectored` & `write_vectored` require handling returned value likewise non-vectored methods. rust-lang/rust#68041

---

changelog: lint vectored IO in `unused_io_amount` lint
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants