-
Notifications
You must be signed in to change notification settings - Fork 13.7k
Ensure non-empty buffers for large vectored I/O #138879
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
Open
+122
−33
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1626,6 +1626,60 @@ impl<'a> Deref for IoSlice<'a> { | |
| } | ||
| } | ||
|
|
||
| /// Limits a slice of buffers to at most `n` buffers and ensures that it has at | ||
| /// least one buffer, even if empty. | ||
| /// | ||
| /// When the slice contains over `n` buffers, ensure that at least one non-empty | ||
| /// buffer is in the truncated slice, if there is one. | ||
| #[allow(unused_macros)] // Not used on all platforms | ||
| pub(crate) macro limit_slices($bufs:expr, $n:expr) { | ||
| 'slices: { | ||
| let bufs: &[IoSlice<'_>] = $bufs; | ||
| let n: usize = $n; | ||
| super let empty = &[IoSlice::new(&[])]; | ||
| // if bufs.len() > n || bufs.is_empty() | ||
| if core::intrinsics::unlikely(bufs.len().wrapping_sub(1) >= n) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you use Also the
Comment on lines
+1636
to
+1641
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry if I missed this in the other comments, but why is the block required here? It seems like assigning to an let bufs: &[IoSlice<'_>] = $bufs; let n: usize = $n; let empty = &[IoSlice::new(&[])]; let mut ret = None; if (...) { for (i, buf) ... { if !buf.is_empty() { ret = Some(&bufs[i..i + len]); break; } } } ret.unwrap_or(empty) |
||
| for (i, buf) in bufs.iter().enumerate() { | ||
| if !buf.is_empty() { | ||
| let len = cmp::min(bufs.len() - i, n); | ||
| break 'slices &bufs[i..i + len]; | ||
|
Comment on lines
+1644
to
+1645
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For clarity:
Suggested change
let len = cmp::min(bufs.len() - i, n);
break 'slices &bufs[i..i + len];
// Take all remaining buffers, clamped to `n` items
let len = cmp::min(bufs.len() - i, n);
break 'slices &bufs[i..(i + len)];
|
||
| } | ||
| } | ||
| // POSIX requires at least one buffer for writev. | ||
| // https://pubs.opengroup.org/onlinepubs/9799919799/functions/writev.html | ||
| break 'slices empty; | ||
| } | ||
| bufs | ||
| } | ||
| } | ||
|
|
||
| /// Limits a slice of buffers to at most `n` buffers and ensures that it has at | ||
| /// least one buffer, even if empty. | ||
| /// | ||
| /// When the slice contains over `n` buffers, ensure that at least one non-empty | ||
| /// buffer is in the truncated slice, if there is one. | ||
| #[allow(unused_macros)] // Not used on all platforms | ||
| pub(crate) macro limit_slices_mut($bufs:expr, $n:expr) { | ||
| 'slices: { | ||
| let bufs: &mut [IoSliceMut<'_>] = $bufs; | ||
| let n: usize = $n; | ||
| super let empty = &mut [IoSliceMut::new(&mut [])]; | ||
| // if bufs.len() > n || bufs.is_empty() | ||
| if core::intrinsics::unlikely(bufs.len().wrapping_sub(1) >= n) { | ||
| for (i, buf) in bufs.iter().enumerate() { | ||
| if !buf.is_empty() { | ||
| let len = cmp::min(bufs.len() - i, n); | ||
| break 'slices &mut bufs[i..i + len]; | ||
| } | ||
| } | ||
| // POSIX requires at least one buffer for readv. | ||
| // https://pubs.opengroup.org/onlinepubs/9799919799/functions/readv.html | ||
| break 'slices empty; | ||
| } | ||
| bufs | ||
| } | ||
| } | ||
|
|
||
| /// A trait for objects which are byte-oriented sinks. | ||
| /// | ||
| /// Implementors of the `Write` trait are sometimes called 'writers'. | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 23 additions & 3 deletions
library/std/src/sys/fd/unix/tests.rs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the Windows tests be updated too?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,12 +1,32 @@ | ||
| use core::mem::ManuallyDrop; | ||
|
|
||
| use super::FileDesc; | ||
| use super::{FileDesc, max_iov}; | ||
| use crate::io::IoSlice; | ||
| use crate::os::unix::io::FromRawFd; | ||
|
|
||
| #[test] | ||
| fn limit_vector_count() { | ||
| const IOV_MAX: usize = max_iov(); | ||
|
|
||
| let stdout = ManuallyDrop::new(unsafe { FileDesc::from_raw_fd(1) }); | ||
| let mut bufs = vec![IoSlice::new(&[]); IOV_MAX * 2 + 1]; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that IOV_MAX can be pretty large in theory. Maybe it would be good to panic on platforms where the value is large (tens of Mb)? That would give us a heads up to skip the test on platforms where the allocation would eat a chunk of memory or possibly OOM. |
||
| assert_eq!(stdout.write_vectored(&bufs).unwrap(), 0); | ||
|
|
||
| // The slice of buffers is truncated to IOV_MAX buffers. However, since the | ||
| // first IOV_MAX buffers are all empty, it is sliced starting at the first | ||
| // non-empty buffer to avoid erroneously returning Ok(0). In this case, that | ||
| // starts with the b"hello" buffer and ends just before the b"world!" | ||
| // buffer. | ||
| bufs[IOV_MAX] = IoSlice::new(b"hello"); | ||
| bufs[IOV_MAX * 2] = IoSlice::new(b"world!"); | ||
| assert_eq!(stdout.write_vectored(&bufs).unwrap(), b"hello".len()) | ||
| } | ||
|
|
||
| #[test] | ||
| fn empty_vector() { | ||
| let stdin = ManuallyDrop::new(unsafe { FileDesc::from_raw_fd(0) }); | ||
| assert_eq!(stdin.read_vectored(&mut []).unwrap(), 0); | ||
|
|
||
| let stdout = ManuallyDrop::new(unsafe { FileDesc::from_raw_fd(1) }); | ||
| let bufs = (0..1500).map(|_| IoSlice::new(&[])).collect::<Vec<_>>(); | ||
| assert!(stdout.write_vectored(&bufs).is_ok()); | ||
| assert_eq!(stdout.write_vectored(&[]).unwrap(), 0); | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.