This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8213

rust-pathauditor: use interior mutability for use in multi-threaded contexts
ClosedPublic

Authored by Alphare on Mar 4 2020, 9:45 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG07d9fd6097e6: rust-pathauditor: use interior mutability for use in multi-threaded contexts
Summary

The usual recommendation for using RwLock or Mutex is that if there are
about as many write as there are reads, use Mutex, and if there are more
reads than writes, use RwLock.

If after the main bottleneck (i.e. parallel traversal) is removed this shows
up on profiles, we should investigate using the parking_lot since we don't
need a poisoning API, or maybe move to different types of caches entirely.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Alphare created this revision.Mar 4 2020, 9:45 AM
Alphare updated this revision to Diff 20449.Mar 4 2020, 9:57 AM
Alphare updated this revision to Diff 20456.Mar 4 2020, 10:08 AM
Alphare updated this revision to Diff 20499.Mar 5 2020, 3:34 AM
Alphare added a commit: rHG07d9fd6097e6: rust-pathauditor: use interior mutability for use in multi-threaded contexts.Mar 11 2020, 12:25 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG07d9fd6097e6: rust-pathauditor: use interior mutability for use in multi-threaded contexts (authored by Alphare). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 20728

rust/hg-core/src/dirstate/status.rs

})) }))
}; };
let mut to_visit: Vec<_> = to_visit.collect(); let mut to_visit: Vec<_> = to_visit.collect();
to_visit.sort_by(|a, b| a.0.cmp(&b.0)); to_visit.sort_by(|a, b| a.0.cmp(&b.0));
// We walked all dirs under the roots that weren't ignored, and // We walked all dirs under the roots that weren't ignored, and
// everything that matched was stat'ed and is already in results. // everything that matched was stat'ed and is already in results.
// The rest must thus be ignored or under a symlink. // The rest must thus be ignored or under a symlink.
let mut path_auditor = PathAuditor::new(root_dir); let path_auditor = PathAuditor::new(root_dir);
for (ref filename, entry) in to_visit { for (ref filename, entry) in to_visit {
// Report ignored items in the dmap as long as they are not // Report ignored items in the dmap as long as they are not
// under a symlink directory. // under a symlink directory.
if path_auditor.check(filename) { if path_auditor.check(filename) {
// TODO normalize for case-insensitive filesystems // TODO normalize for case-insensitive filesystems
let buf = hg_path_to_path_buf(filename)?; let buf = hg_path_to_path_buf(filename)?;
results.insert( results.insert(

rust/hg-core/src/utils/files.rs

let cwd = cwd.as_ref(); let cwd = cwd.as_ref();
let name = name.as_ref(); let name = name.as_ref();
let name = if !name.is_absolute() { let name = if !name.is_absolute() {
root.join(&cwd).join(&name) root.join(&cwd).join(&name)
} else { } else {
name.to_owned() name.to_owned()
}; };
let mut auditor = PathAuditor::new(&root); let auditor = PathAuditor::new(&root);
if name != root && name.starts_with(&root) { if name != root && name.starts_with(&root) {
let name = name.strip_prefix(&root).unwrap(); let name = name.strip_prefix(&root).unwrap();
auditor.audit_path(path_to_hg_path_buf(name)?)?; auditor.audit_path(path_to_hg_path_buf(name)?)?;
return Ok(name.to_owned()); return Ok(name.to_owned());
} else if name == root { } else if name == root {
return Ok("".into()); return Ok("".into());
} else { } else {
// Determine whether `name' is in the hierarchy at or beneath `root', // Determine whether `name' is in the hierarchy at or beneath `root',

rust/hg-core/src/utils/path_auditor.rs

// path_auditor.rs // path_auditor.rs
// //
// Copyright 2020 // Copyright 2020
// Raphaël Gomès <rgomes@octobus.net>, // Raphaël Gomès <rgomes@octobus.net>,
// //
// This software may be used and distributed according to the terms of the // This software may be used and distributed according to the terms of the
// GNU General Public License version 2 or any later version. // GNU General Public License version 2 or any later version.
use crate::utils::{ use crate::utils::{
files::lower_clean, files::lower_clean,
find_slice_in_slice, find_slice_in_slice,
hg_path::{hg_path_to_path_buf, HgPath, HgPathBuf, HgPathError}, hg_path::{hg_path_to_path_buf, HgPath, HgPathBuf, HgPathError},
}; };
use std::collections::HashSet; use std::collections::HashSet;
use std::path::{Path, PathBuf}; use std::path::{Path, PathBuf};
use std::sync::{Mutex, RwLock};
/// Ensures that a path is valid for use in the repository i.e. does not use /// Ensures that a path is valid for use in the repository i.e. does not use
/// any banned components, does not traverse a symlink, etc. /// any banned components, does not traverse a symlink, etc.
#[derive(Debug, Default)] #[derive(Debug, Default)]
pub struct PathAuditor { pub struct PathAuditor {
audited: HashSet<HgPathBuf>, audited: Mutex<HashSet<HgPathBuf>>,
audited_dirs: HashSet<HgPathBuf>, audited_dirs: RwLock<HashSet<HgPathBuf>>,
root: PathBuf, root: PathBuf,
} }
impl PathAuditor { impl PathAuditor {
pub fn new(root: impl AsRef<Path>) -> Self { pub fn new(root: impl AsRef<Path>) -> Self {
Self { Self {
root: root.as_ref().to_owned(), root: root.as_ref().to_owned(),
..Default::default() ..Default::default()
} }
} }
pub fn audit_path( pub fn audit_path(
&mut self, &self,
path: impl AsRef<HgPath>, path: impl AsRef<HgPath>,
) -> Result<(), HgPathError> { ) -> Result<(), HgPathError> {
// TODO windows "localpath" normalization // TODO windows "localpath" normalization
let path = path.as_ref(); let path = path.as_ref();
if path.is_empty() { if path.is_empty() {
return Ok(()); return Ok(());
} }
// TODO case normalization // TODO case normalization
if self.audited.contains(path) { if self.audited.lock().unwrap().contains(path) {
return Ok(()); return Ok(());
} }
// AIX ignores "/" at end of path, others raise EISDIR. // AIX ignores "/" at end of path, others raise EISDIR.
let last_byte = path.as_bytes()[path.len() - 1]; let last_byte = path.as_bytes()[path.len() - 1];
if last_byte == b'/' || last_byte == b'\\' { if last_byte == b'/' || last_byte == b'\\' {
return Err(HgPathError::EndsWithSlash(path.to_owned())); return Err(HgPathError::EndsWithSlash(path.to_owned()));
} }
let parts: Vec<_> = path let parts: Vec<_> = path
// We don't want to add "foo/bar/baz" to `audited_dirs` before checking // We don't want to add "foo/bar/baz" to `audited_dirs` before checking
// if there's a "foo/.hg" directory. This also means we won't // if there's a "foo/.hg" directory. This also means we won't
// accidentally traverse a symlink into some other filesystem (which // accidentally traverse a symlink into some other filesystem (which
// is potentially expensive to access). // is potentially expensive to access).
for index in 0..parts.len() { for index in 0..parts.len() {
let prefix = &parts[..index + 1].join(&b'/'); let prefix = &parts[..index + 1].join(&b'/');
let prefix = HgPath::new(prefix); let prefix = HgPath::new(prefix);
if self.audited_dirs.contains(prefix) { if self.audited_dirs.read().unwrap().contains(prefix) {
continue; continue;
} }
self.check_filesystem(&prefix, &path)?; self.check_filesystem(&prefix, &path)?;
self.audited_dirs.insert(prefix.to_owned()); self.audited_dirs.write().unwrap().insert(prefix.to_owned());
} }
self.audited.insert(path.to_owned()); self.audited.lock().unwrap().insert(path.to_owned());
Ok(()) Ok(())
} }
pub fn check_filesystem( pub fn check_filesystem(
&self, &self,
prefix: impl AsRef<HgPath>, prefix: impl AsRef<HgPath>,
path: impl AsRef<HgPath>, path: impl AsRef<HgPath>,
}); });
} }
} }
}; };
Ok(()) Ok(())
} }
pub fn check(&mut self, path: impl AsRef<HgPath>) -> bool { pub fn check(&self, path: impl AsRef<HgPath>) -> bool {
self.audit_path(path).is_ok() self.audit_path(path).is_ok()
} }
} }
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::*; use super::*;
use crate::utils::files::get_path_from_bytes; use crate::utils::files::get_path_from_bytes;
use crate::utils::hg_path::path_to_hg_path_buf; use crate::utils::hg_path::path_to_hg_path_buf;
#[test] #[test]
fn test_path_auditor() { fn test_path_auditor() {
let mut auditor = PathAuditor::new(get_path_from_bytes(b"/tmp")); let auditor = PathAuditor::new(get_path_from_bytes(b"/tmp"));
let path = HgPath::new(b".hg/00changelog.i"); let path = HgPath::new(b".hg/00changelog.i");
assert_eq!( assert_eq!(
auditor.audit_path(path), auditor.audit_path(path),
Err(HgPathError::InsideDotHg(path.to_owned())) Err(HgPathError::InsideDotHg(path.to_owned()))
); );
let path = HgPath::new(b"this/is/nested/.hg/thing.txt"); let path = HgPath::new(b"this/is/nested/.hg/thing.txt");
assert_eq!( assert_eq!(