Skip to content

BufWriter remove Option #59759

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

Closed
wants to merge 4 commits into from
Closed

BufWriter remove Option #59759

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6cb3717
BufWriter remove Option around inner Write implementation
czipperz Apr 6, 2019
fadb8dc
Add benchmarks
czipperz Apr 6, 2019
d1fe13d
Use read and drop_in_place instead of replace and uninitialized
czipperz Apr 6, 2019
d3311b7
Fix get_ref and get_mut implementations
czipperz Apr 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions src/libstd/io/buffered.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -410,7 +410,7 @@ impl<R: Seek> Seek for BufReader<R> {
/// [`flush`]: #method.flush
#[stable(feature = "rust1", since = "1.0.0")]
pub struct BufWriter<W: Write> {
inner: Option<W>,
inner: W,
buf: Vec<u8>,
// #30888: If the inner writer panics in a call to write, we don't want to
// write the buffered data a second time in BufWriter's destructor. This
Expand Down Expand Up @@ -479,7 +479,7 @@ impl<W: Write> BufWriter<W> {
#[stable(feature = "rust1", since = "1.0.0")]
pub fn with_capacity(cap: usize, inner: W) -> BufWriter<W> {
BufWriter {
inner: Some(inner),
inner,
buf: Vec::with_capacity(cap),
panicked: false,
}
Expand All @@ -491,7 +491,7 @@ impl<W: Write> BufWriter<W> {
let mut ret = Ok(());
while written < len {
self.panicked = true;
let r = self.inner.as_mut().unwrap().write(&self.buf[written..]);
let r = self.inner.write(&self.buf[written..]);
self.panicked = false;

match r {
Expand Down Expand Up @@ -526,7 +526,7 @@ impl<W: Write> BufWriter<W> {
/// let reference = buffer.get_ref();
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn get_ref(&self) -> &W { self.inner.as_ref().unwrap() }
pub fn get_ref(&self) -> &W { &self.inner }

/// Gets a mutable reference to the underlying writer.
///
Expand All @@ -544,7 +544,7 @@ impl<W: Write> BufWriter<W> {
/// let reference = buffer.get_mut();
/// ```
#[stable(feature = "rust1", since = "1.0.0")]
pub fn get_mut(&mut self) -> &mut W { self.inner.as_mut().unwrap() }
pub fn get_mut(&mut self) -> &mut W { &mut self.inner }

/// Returns a reference to the internally buffered data.
///
Expand Down Expand Up @@ -588,7 +588,14 @@ impl<W: Write> BufWriter<W> {
pub fn into_inner(mut self) -> Result<W, IntoInnerError<BufWriter<W>>> {
match self.flush_buf() {
Err(e) => Err(IntoInnerError(self, e)),
Ok(()) => Ok(self.inner.take().unwrap())
Ok(()) => {
use crate::mem::forget;
use crate::ptr::{drop_in_place, read};
let inner = unsafe { read(&mut self.inner) };
Copy link
Member

Choose a reason for hiding this comment

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

This approach of ~manually running the drop glue for all of the other parts of the struct seems a bit dangerous and error prone. Using ManuallyDrop::take seems like it'd be cleaner: https://doc.rust-lang.org/std/mem/struct.ManuallyDrop.html?search=#method.take

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What are you suggesting applying ManuallyDrop::take in this context?

Copy link
Member

Choose a reason for hiding this comment

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

I think you would need the struct to contain:

    inner: ManuallyDrop<W>,
    buf: ManuallyDrop<Vec<u8>>,

where you call ManuallyDrop::drop(&mut self.buf); ManuallyDrop::drop(&mut self.inner) inside Drop and ManuallyDrop::drop(&mut self.buf); ManuallyDrop::take(&mut self.inner) inside into_inner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like that implementation at all. The entire point of this change is to isolate this edge case into one function rather than spreading it throughout the entire struct.

Copy link
Contributor Author

@czipperz czipperz Apr 6, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Perhaps he meant putting self into a ManuallyDrop to prevent these double free issues? That seems reasonable:

use crate::mem::ManuallyDrop;
use crate::ptr::{drop_in_place, read};
let mut s = ManuallyDrop::new(self);
let inner = unsafe { read(&mut s.inner) };
unsafe { drop_in_place(&mut s.buf) };
Ok(inner)

unsafe { drop_in_place(&mut self.buf) };
Copy link
Member

Choose a reason for hiding this comment

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

I notice that if you have a global memory allocator that can panic in dealloc, a panic here would turn into a double-free of inner and self.inner. The GlobalAlloc trait is currently documented as:

  • It's undefined behavior if global allocators unwind. This restriction may be lifted in the future, but currently a panic from any of these functions may lead to memory unsafety.

so this seems fine for now but needs at least a comment why the code is safe despite appearances.

forget(self);
Ok(inner)
}
}
}
}
Expand All @@ -601,7 +608,7 @@ impl<W: Write> Write for BufWriter<W> {
}
if buf.len() >= self.buf.capacity() {
self.panicked = true;
let r = self.inner.as_mut().unwrap().write(buf);
let r = self.inner.write(buf);
self.panicked = false;
r
} else {
Expand All @@ -616,7 +623,7 @@ impl<W: Write> Write for BufWriter<W> {
}
if total_len >= self.buf.capacity() {
self.panicked = true;
let r = self.inner.as_mut().unwrap().write_vectored(bufs);
let r = self.inner.write_vectored(bufs);
self.panicked = false;
r
} else {
Expand All @@ -633,7 +640,7 @@ impl<W: Write> Write for BufWriter<W> {
impl<W: Write> fmt::Debug for BufWriter<W> where W: fmt::Debug {
fn fmt(&self, fmt: &mut fmt::Formatter<'_>) -> fmt::Result {
fmt.debug_struct("BufWriter")
.field("writer", &self.inner.as_ref().unwrap())
.field("writer", &self.inner)
.field("buffer", &format_args!("{}/{}", self.buf.len(), self.buf.capacity()))
.finish()
}
Expand All @@ -652,7 +659,7 @@ impl<W: Write + Seek> Seek for BufWriter<W> {
#[stable(feature = "rust1", since = "1.0.0")]
impl<W: Write> Drop for BufWriter<W> {
fn drop(&mut self) {
if self.inner.is_some() && !self.panicked {
if !self.panicked {
// dtors should not panic, so we ignore a failed flush
let _r = self.flush_buf();
}
Expand Down Expand Up @@ -1392,6 +1399,32 @@ mod tests {
});
}

#[bench]
fn bench_buffered_writer_large_write_causing_flush(b: &mut test::Bencher) {
b.iter(|| {
let mut buf_writer = BufWriter::with_capacity(10, io::sink());
buf_writer.write(b"abcdefghijklmnopqrstuvwxyz").unwrap();
});
}

#[bench]
fn bench_buffered_writer_small_write(b: &mut test::Bencher) {
b.iter(|| {
let mut buf_writer = BufWriter::with_capacity(10, io::sink());
buf_writer.write(b"abcdef").unwrap();
});
}

#[bench]
fn bench_buffered_writer_multiple_small_writes(b: &mut test::Bencher) {
b.iter(|| {
let mut buf_writer = BufWriter::with_capacity(10, io::sink());
buf_writer.write(b"abcdef").unwrap();
buf_writer.write(b"abcdef").unwrap();
buf_writer.write(b"abcdef").unwrap();
});
}

struct AcceptOneThenFail {
written: bool,
flushed: bool,
Expand Down