Skip to content

Commit

Permalink
Make WorkspaceStore::retrieve_path_from_id returns the entry manife…
Browse files Browse the repository at this point in the history 
…st to improve concurrency hardening
  • Loading branch information
@touilleMan
touilleMan committed Sep 15, 2024
1 parent 01169ad commit c062528
Show file tree
Hide file tree
Showing 5 changed files with 89 additions and 99 deletions.
4 changes: 2 additions & 2 deletions libparsec/crates/client/src/workspace/store/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,10 +211,10 @@ impl WorkspaceStore {
resolve_path::resolve_path(self, path).await
}

pub(crate) async fn retrieve_path_from_id(
pub async fn retrieve_path_from_id(
&self,
entry_id: VlobID,
) -> Result<(FsPath, PathConfinementPoint), ResolvePathError> {
) -> Result<(ArcLocalChildManifest, FsPath, PathConfinementPoint), ResolvePathError> {
resolve_path::retrieve_path_from_id(self, entry_id).await
}

Expand Down
20 changes: 11 additions & 9 deletions libparsec/crates/client/src/workspace/store/resolve_path.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -660,7 +660,7 @@ pub(crate) async fn resolve_path_for_reparenting(
}

enum CacheOnlyRetrievalOutcome {
Done(FsPath, PathConfinementPoint),
Done((ArcLocalChildManifest, FsPath, PathConfinementPoint)),
EntryNotFound,
NeedPopulateCache(VlobID),
}
Expand All @@ -676,12 +676,14 @@ fn cache_only_retrieve_path_from_id(
// Get the root ID
let root_entry_id = cache.manifests.root_manifest().base.id;

let entry_manifest = match cache.manifests.get(&entry_id) {
Some(manifest) => manifest.clone(),
None => return CacheOnlyRetrievalOutcome::NeedPopulateCache(entry_id),
};

// Initialize the loop state
let mut current_entry_id = entry_id;
let mut current_parent_id = match cache.manifests.get(&current_entry_id) {
Some(manifest) => manifest.parent(),
None => return CacheOnlyRetrievalOutcome::NeedPopulateCache(current_entry_id),
};
let mut current_parent_id = entry_manifest.parent();
let mut seen: HashSet<VlobID> = HashSet::from_iter([current_entry_id]);

// Loop until we reach the root
Expand Down Expand Up @@ -731,14 +733,14 @@ fn cache_only_retrieve_path_from_id(

// Reverse the parts to get the path
parts.reverse();
CacheOnlyRetrievalOutcome::Done(FsPath::from_parts(parts), confinement)
CacheOnlyRetrievalOutcome::Done((entry_manifest, FsPath::from_parts(parts), confinement))
}

/// Retrieve the path and the confinement point of a given entry ID.
pub(crate) async fn retrieve_path_from_id(
store: &super::WorkspaceStore,
entry_id: VlobID,
) -> Result<(FsPath, PathConfinementPoint), ResolvePathError> {
) -> Result<(ArcLocalChildManifest, FsPath, PathConfinementPoint), ResolvePathError> {
loop {
// Most of the time we should have each entry in the path already in the cache,
// so we want to lock the cache once and only release it in the unlikely case
Expand All @@ -748,8 +750,8 @@ pub(crate) async fn retrieve_path_from_id(
cache_only_retrieve_path_from_id(&mut cache, entry_id)
};
match cache_only_outcome {
CacheOnlyRetrievalOutcome::Done(path, confinement_point) => {
return Ok((path, confinement_point))
CacheOnlyRetrievalOutcome::Done((entry_manifest, path, confinement_point)) => {
return Ok((entry_manifest, path, confinement_point))
}
CacheOnlyRetrievalOutcome::EntryNotFound => {
return Err(ResolvePathError::EntryNotFound)
Expand Down
46 changes: 11 additions & 35 deletions libparsec/crates/client/src/workspace/transactions/read_folder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,7 @@ use libparsec_types::prelude::*;
use crate::{
certif::{InvalidCertificateError, InvalidKeysBundleError, InvalidManifestError},
workspace::{
store::{
EnsureManifestExistsWithParentError, GetManifestError, PathConfinementPoint,
ResolvePathError,
},
store::{EnsureManifestExistsWithParentError, PathConfinementPoint, ResolvePathError},
EntryStat, WorkspaceOps, WorkspaceStatEntryError,
},
};
Expand Down Expand Up @@ -229,48 +226,27 @@ pub async fn open_folder_reader_by_id(
ops: &WorkspaceOps,
entry_id: VlobID,
) -> Result<FolderReader, WorkspaceOpenFolderReaderError> {
let manifest = ops
let (manifest, _, confinement_point) = ops
.store
.get_manifest(entry_id)
.retrieve_path_from_id(entry_id)
.await
.map_err(|err| match err {
GetManifestError::Offline => WorkspaceOpenFolderReaderError::Offline,
GetManifestError::Stopped => WorkspaceOpenFolderReaderError::Stopped,
GetManifestError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound,
GetManifestError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess,
GetManifestError::InvalidKeysBundle(err) => {
ResolvePathError::Offline => WorkspaceOpenFolderReaderError::Offline,
ResolvePathError::Stopped => WorkspaceOpenFolderReaderError::Stopped,
ResolvePathError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound,
ResolvePathError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess,
ResolvePathError::InvalidKeysBundle(err) => {
WorkspaceOpenFolderReaderError::InvalidKeysBundle(err)
}
GetManifestError::InvalidCertificate(err) => {
ResolvePathError::InvalidCertificate(err) => {
WorkspaceOpenFolderReaderError::InvalidCertificate(err)
}
GetManifestError::InvalidManifest(err) => {
ResolvePathError::InvalidManifest(err) => {
WorkspaceOpenFolderReaderError::InvalidManifest(err)
}
GetManifestError::Internal(err) => err.context("cannot get manifest").into(),
ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(),
})?;

let (_, confinement_point) =
ops.store
.retrieve_path_from_id(entry_id)
.await
.map_err(|err| match err {
ResolvePathError::Offline => WorkspaceOpenFolderReaderError::Offline,
ResolvePathError::Stopped => WorkspaceOpenFolderReaderError::Stopped,
ResolvePathError::EntryNotFound => WorkspaceOpenFolderReaderError::EntryNotFound,
ResolvePathError::NoRealmAccess => WorkspaceOpenFolderReaderError::NoRealmAccess,
ResolvePathError::InvalidKeysBundle(err) => {
WorkspaceOpenFolderReaderError::InvalidKeysBundle(err)
}
ResolvePathError::InvalidCertificate(err) => {
WorkspaceOpenFolderReaderError::InvalidCertificate(err)
}
ResolvePathError::InvalidManifest(err) => {
WorkspaceOpenFolderReaderError::InvalidManifest(err)
}
ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(),
})?;

match manifest {
ArcLocalChildManifest::Folder(manifest) => Ok(FolderReader {
manifest,
Expand Down
89 changes: 45 additions & 44 deletions libparsec/crates/client/src/workspace/transactions/stat_entry.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,51 +86,52 @@ pub(crate) async fn stat_entry_by_id(
entry_id: VlobID,
maybe_known_confinement_point: Option<PathConfinementPoint>,
) -> Result<EntryStat, WorkspaceStatEntryError> {
let manifest = ops
.store
.get_manifest(entry_id)
.await
.map_err(|err| match err {
GetManifestError::Offline => WorkspaceStatEntryError::Offline,
GetManifestError::Stopped => WorkspaceStatEntryError::Stopped,
GetManifestError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound,
GetManifestError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess,
GetManifestError::InvalidKeysBundle(err) => {
WorkspaceStatEntryError::InvalidKeysBundle(err)
}
GetManifestError::InvalidCertificate(err) => {
WorkspaceStatEntryError::InvalidCertificate(err)
}
GetManifestError::InvalidManifest(err) => WorkspaceStatEntryError::InvalidManifest(err),
GetManifestError::Internal(err) => err.context("cannot resolve path").into(),
})?;

let confinement_point = match maybe_known_confinement_point {
Some(known_confinement_point) => known_confinement_point,
let (manifest, confinement_point) = match maybe_known_confinement_point {
Some(known_confinement_point) => {
let manifest = ops
.store
.get_manifest(entry_id)
.await
.map_err(|err| match err {
GetManifestError::Offline => WorkspaceStatEntryError::Offline,
GetManifestError::Stopped => WorkspaceStatEntryError::Stopped,
GetManifestError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound,
GetManifestError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess,
GetManifestError::InvalidKeysBundle(err) => {
WorkspaceStatEntryError::InvalidKeysBundle(err)
}
GetManifestError::InvalidCertificate(err) => {
WorkspaceStatEntryError::InvalidCertificate(err)
}
GetManifestError::InvalidManifest(err) => {
WorkspaceStatEntryError::InvalidManifest(err)
}
GetManifestError::Internal(err) => err.context("cannot resolve path").into(),
})?;
(manifest, known_confinement_point)
}
None => {
let (_, confinement_point) =
ops.store
.retrieve_path_from_id(entry_id)
.await
.map_err(|err| match err {
ResolvePathError::Offline => WorkspaceStatEntryError::Offline,
ResolvePathError::Stopped => WorkspaceStatEntryError::Stopped,
ResolvePathError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound,
ResolvePathError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess,
ResolvePathError::InvalidKeysBundle(err) => {
WorkspaceStatEntryError::InvalidKeysBundle(err)
}
ResolvePathError::InvalidCertificate(err) => {
WorkspaceStatEntryError::InvalidCertificate(err)
}
ResolvePathError::InvalidManifest(err) => {
WorkspaceStatEntryError::InvalidManifest(err)
}
ResolvePathError::Internal(err) => {
err.context("cannot retrieve path").into()
}
})?;
confinement_point
let (manifest, _, confinement_point) = ops
.store
.retrieve_path_from_id(entry_id)
.await
.map_err(|err| match err {
ResolvePathError::Offline => WorkspaceStatEntryError::Offline,
ResolvePathError::Stopped => WorkspaceStatEntryError::Stopped,
ResolvePathError::EntryNotFound => WorkspaceStatEntryError::EntryNotFound,
ResolvePathError::NoRealmAccess => WorkspaceStatEntryError::NoRealmAccess,
ResolvePathError::InvalidKeysBundle(err) => {
WorkspaceStatEntryError::InvalidKeysBundle(err)
}
ResolvePathError::InvalidCertificate(err) => {
WorkspaceStatEntryError::InvalidCertificate(err)
}
ResolvePathError::InvalidManifest(err) => {
WorkspaceStatEntryError::InvalidManifest(err)
}
ResolvePathError::Internal(err) => err.context("cannot retrieve path").into(),
})?;
(manifest, confinement_point)
}
};

Expand Down
29 changes: 20 additions & 9 deletions libparsec/crates/client/tests/unit/workspace/retrieve_path_from_id.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,23 +21,27 @@ async fn ok(env: &TestbedEnv) {
let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await;

// Resolve root
let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap();
let (manifest, path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, "/".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_id);

// Resolve child folder
let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap();
let (manifest, path, confinement) =
ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, "/foo".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_id);

// Resolve child file
let (path, confinement) = ops
let (manifest, path, confinement) = ops
.store
.retrieve_path_from_id(wksp1_foo_egg_txt_id)
.await
.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, "/foo/egg.txt".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_foo_egg_txt_id);
}

#[parsec_test(testbed = "minimal_client_ready")]
Expand Down Expand Up @@ -70,32 +74,37 @@ async fn ok_with_confinement(env: &TestbedEnv) {
.unwrap();

// Resolve root
let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap();
let (manifest, path, confinement) = ops.store.retrieve_path_from_id(wksp1_id).await.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, "/".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_id);

// Resolve foo folder
let (path, confinement) = ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap();
let (manifest, path, confinement) =
ops.store.retrieve_path_from_id(wksp1_foo_id).await.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id));
p_assert_eq!(path, "/foo.tmp".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_id);

// Resolve spam folder
let (path, confinement) = ops
let (manifest, path, confinement) = ops
.store
.retrieve_path_from_id(wksp1_foo_spam_id)
.await
.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id));
p_assert_eq!(path, "/foo.tmp/spam.tmp".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == wksp1_foo_spam_id);

// Resolve egg file
let (path, confinement) = ops
let (manifest, path, confinement) = ops
.store
.retrieve_path_from_id(wksp1_foo_spam_egg_id)
.await
.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::Confined(wksp1_id));
p_assert_eq!(path, "/foo.tmp/spam.tmp/egg.txt".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_foo_spam_egg_id);
}

#[parsec_test(testbed = "minimal_client_ready", with_server)]
Expand Down Expand Up @@ -155,13 +164,14 @@ async fn base_path_mismatch_is_ok(env: &TestbedEnv) {
let alice = env.local_device("alice@dev1");
let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await;

let (path, confinement) = ops
let (manifest, path, confinement) = ops
.store
.retrieve_path_from_id(wksp1_bar_txt_id)
.await
.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, "/bar.txt".parse().unwrap());
p_assert_matches!(manifest, ArcLocalChildManifest::File(manifest) if manifest.base.id == wksp1_bar_txt_id);
}

#[parsec_test(testbed = "minimal_client_ready", with_server)]
Expand Down Expand Up @@ -256,13 +266,14 @@ async fn inconsistent_path_recursive_by_children(
let alice = env.local_device("alice@dev1");
let ops = workspace_ops_factory(&env.discriminant_dir, &alice, wksp1_id.to_owned()).await;

let (path, confinement) = ops
let (manifest, path, confinement) = ops
.store
.retrieve_path_from_id(recursive_target_id)
.await
.unwrap();
p_assert_eq!(confinement, PathConfinementPoint::NotConfined);
p_assert_eq!(path, expected_path);
p_assert_matches!(manifest, ArcLocalChildManifest::Folder(manifest) if manifest.base.id == recursive_target_id);
}

#[parsec_test(testbed = "minimal_client_ready")]
Expand Down

0 comments on commit c062528

Please sign in to comment.