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

feat(byteview): Add non-consuming mmap constructor #448

Merged
merged 4 commits into from
Nov 18, 2021
Merged
Changes from 2 commits
Commits
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
52 changes: 50 additions & 2 deletions symbolic-common/src/byteview.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ impl<'a> ByteView<'a> {

/// Constructs a `ByteView` from an open file handle by memory mapping the file.
///
/// See [`ByteView::mmap_file`] for a non-consuming version of this constructor.
///
/// # Example
///
/// ```
Expand All @@ -131,7 +133,28 @@ impl<'a> ByteView<'a> {
/// }
/// ```
pub fn map_file(file: File) -> Result<Self, io::Error> {
let backing = match unsafe { Mmap::map(&file) } {
Self::mmap_file(&file)
}

/// Constructs a `ByteView` from an open file handle by memory mapping the file.
///
/// The main difference with [`ByteView::map_file`] is that this takes the [`File`] by
/// reference rather than consuming it.
///
/// # Example
///
/// ```
/// use std::io::Write;
/// use symbolic_common::ByteView;
///
/// fn main() -> Result<(), std::io::Error> {
/// let mut file = tempfile::tempfile()?;
/// let view = ByteView::mmap_file(&file)?;
/// Ok(())
/// }
/// ```
pub fn mmap_file(file: &File) -> Result<Self, io::Error> {
let backing = match unsafe { Mmap::map(file) } {
Ok(mmap) => ByteViewBacking::Mmap(mmap),
Err(err) => {
// this is raised on empty mmaps which we want to ignore. The 1006 Windows error
Expand Down Expand Up @@ -234,7 +257,7 @@ unsafe impl StableDeref for ByteView<'_> {}
mod tests {
use super::*;

use std::io::Write;
use std::io::{Read, Seek, Write};

use similar_asserts::assert_eq;
use tempfile::NamedTempFile;
Expand All @@ -260,4 +283,29 @@ mod tests {

Ok(())
}

#[test]
fn test_mmap_fd_reuse() -> Result<(), std::io::Error> {
let mut tmp = NamedTempFile::new()?;
tmp.write_all(b"1234")?;

let view = ByteView::mmap_file(tmp.as_file())?;

// This deletes the file on disk.
let path = tmp.path().to_path_buf();
let mut file = tmp.into_file();
assert!(!path.exists());

// Ensure we can still read from the the file after mmapping and deleting it on disk.
let mut buf = Vec::new();
file.rewind()?;
file.read_to_end(&mut buf)?;
Copy link
Member

Choose a reason for hiding this comment

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

So does mmap-ing keep the FD alive? Might be interesting to explicitly drop(file) after this, and see that the view still works (and holds on to the FD).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, mmap only uses the fd to figure out what to map, afterwards you can close it or do whatever you like to it. I added the explicit drop to make it even more obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes,

That "yes" is not really true as the rest of the sentence explains 😄 mmap does not use the fd at all other than finding the right file to map. so it doesn't keep the FD alive either.

assert_eq!(buf, b"1234");
drop(file);

// Ensure the byteview can still read the file as well.
assert_eq!(&*view, b"1234");

Ok(())
}
}