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

Improve documentation of read_vectored and write_vectored #74825

Open
kubkon opened this issue Jul 27, 2020 · 11 comments
Open

Improve documentation of read_vectored and write_vectored #74825

kubkon opened this issue Jul 27, 2020 · 11 comments
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Comments

@kubkon
Copy link
Contributor

kubkon commented Jul 27, 2020

Apologies if this is a duplicate, but I couldn't find a mention of this anywhere.

A simple snippet like the following:

    let mut file = File::open("test.txt")?;

    let buffer = &mut [0u8; 6];
    let (mut first, mut second) = buffer.split_at_mut(3);
    let iovecs = &mut [IoSliceMut::new(&mut first), IoSliceMut::new(&mut second)];
    file.read_vectored(iovecs)?;

will incorrectly only populate the first IoSliceMut and exit prematurely on Windows. (See here for a full snippet).

See bytecodealliance/wasmtime#2070 for a downstream issue.

@kubkon kubkon added the C-bug Category: This is a bug. label Jul 27, 2020
@kubkon
Copy link
Contributor Author

kubkon commented Jul 27, 2020

cc @ueno

@jonas-schievink jonas-schievink added T-libs Relevant to the library team, which will review and decide on the PR/issue. O-windows Operating system: Windows labels Jul 27, 2020
@nagisa
Copy link
Member

nagisa commented Jul 27, 2020

Much like with read, you must inspect the number of bytes that read_vectored returns. That’s the number of bytes read, and the buffers are filled in as if they were concatenated. In your case, I’m almost certain that the function call returned 3.

The documentation also says this:

Data is copied to fill each buffer in order, with the final buffer written to possibly being only partially filled. This method must behave equivalently to a single call to read with concatenated buffers.

@kubkon
Copy link
Contributor Author

kubkon commented Jul 27, 2020

Hey @nagisa, thanks for this! I agree with what you say, it's just that the behaviour is inconsistent between the platforms. On macOS/Linux for instance, read_vectored correctly returns 6 bytes read, whereas on Windows only 3. In fact, it turns out it doesn't matter how many IoSliceMut chunks you supply, it will always only fill in the first one. So if you modify the example above to two splits on buffer into chunks of length 2, you will only get the first chunk filled.

@kubkon
Copy link
Contributor Author

kubkon commented Jul 27, 2020

I should also add, that if this is the intended behaviour on Windows, then that's fine but it should be documented more clearly. This line in the docs makes it sound as if this should return all slices filled:

This method must behave equivalently to a single call to read with concatenated buffers.

So I guess a sentence on expected differences between hosts would go a long way here.

@retep998
Copy link
Member

retep998 commented Jul 28, 2020

Any platform is free to fill as much or as little of your provided buffer(s) as they want, and does not have to be consistent about it. It is your responsibility, regardless of platform, to check how many bytes were read and proceed accordingly.

That said, Windows does not have vectored read/write operations for anything other than sockets, so aside from sockets read_vectored and write_vectored currently will only look at the first non-empty buffer on Windows. However, std does reserve the right to change this behavior in the future, especially if Windows does gain an API for vectored file reads/writes.

Better documentation on this would be a good idea for sure.

@kubkon
Copy link
Contributor Author

kubkon commented Jul 28, 2020

@retep998 agreed! Thanks for the input and explanation.

I’ll be more than happy to help documenting this, but I’m wondering what the best next steps wrt this issue are. Given that this is not a bug I guess it’ll make sense to close this issue. If you feel otherwise, lemme know and I’ll reopen or feel free to do this yourself!

@kubkon kubkon closed this as completed Jul 28, 2020
@retep998
Copy link
Member

Generally when an issue turns out to be not a bug, but docs can be improved, the issue is left open but tags changed like so.

@retep998 retep998 reopened this Jul 28, 2020
@retep998 retep998 added A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed C-bug Category: This is a bug. O-windows Operating system: Windows labels Jul 28, 2020
@retep998 retep998 changed the title Incorrect read_vectored behaviour on Windows Improve documentation of read_vectored and write_vectored Jul 28, 2020
@pickfire
Copy link
Contributor

Any idea on how to improve the docs for this?

@poliorcetics
Copy link
Contributor

Maybe something like the std::time::Instant documentation, with a section about OS-specific behaviour ?

@pickfire
Copy link
Contributor

pickfire commented Sep 6, 2020

Do you think these are good example for Vectored I/O? I try my best to show a good use case, feel free to give any suggestions.

use std::io::{IoSlice, Write};

let mut writer = Vec::new();
let buf = [
    IoSlice::new(b"GET "),
    IoSlice::new(&[47]),
    IoSlice::new(b" HTTP/1.1"),
];
assert_eq!(writer.write_vectored(&buf).unwrap(), 14);
assert_eq!(writer, b"GET / HTTP/1.1");
use std::io::{IoSliceMut, Read};

// Capn' Proto recommends framing in 4 bytes
let reader = vec![0_u8, 0, 0, 0, 1, 2, 3, 4];
let mut buf = vec![0; 8];
let mut iovecs = buf
    .chunks_exact_mut(4)
    .map(IoSliceMut::new)
    .collect::<Vec<_>>();
assert_eq!(reader.as_slice().read_vectored(&mut iovecs).unwrap(), 8);
assert_eq!(buf, reader);

I think from the documentation side we need to add both examples and OS-specific behavior for this (I don't know much about these for Vectored I/O).

@elidupree
Copy link

Today I just happened to be reading the documentation for write_vectored, which currently states:

This method must behave as a call to write with the buffers concatenated would.

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

This appears to describe a trait contract, and then immediately violate it!

Even after reading this thread, I am left moderately confused about the purpose of this method or the obligations of a Write implementor in relation to it. Better docs are definitely in order.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools C-enhancement Category: An issue proposing an enhancement or a PR with one. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants