From ecf9ddff1078f347ebf7bb537bc7bef4c7296576 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Fri, 1 Mar 2024 12:03:01 -0500 Subject: [PATCH 1/4] Replace custom Drop impl for UnpublishedOperation with #[must_use] The custom Drop impl prevents us from moving members of UnpublishedOperation, and is the reason why `NewRepoData` is wrapped in an `Option`. We don't use custom Drop functions like this for debugging elsewhere in the codebase, and in some ways #[must_use] provides better protection since it will typically cause a compiler error if the UnpublishedOperation isn't used. --- lib/src/transaction.rs | 36 ++++++++++++------------------------ 1 file changed, 12 insertions(+), 24 deletions(-) diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 9145da348b..062d0a0c0d 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -153,10 +153,16 @@ struct NewRepoData { index: Box, } +/// An Operation which has been written to the operation store but not +/// published. The repo can be loaded at an unpublished Operation, but the +/// Operation will not be visible in the op log if the repo is loaded at head. +/// +/// Either [`Self::publish`] or [`Self::leave_unpublished`] must be called to +/// finish the operation. +#[must_use = "Either publish() or leave_unpublished() must be called to finish the operation."] pub struct UnpublishedOperation { repo_loader: RepoLoader, data: Option, - closed: bool, } impl UnpublishedOperation { @@ -171,11 +177,7 @@ impl UnpublishedOperation { view, index, }); - UnpublishedOperation { - repo_loader, - data, - closed: false, - } + UnpublishedOperation { repo_loader, data } } pub fn operation(&self) -> &Operation { @@ -190,27 +192,13 @@ impl UnpublishedOperation { .op_heads_store() .update_op_heads(data.operation.parent_ids(), data.operation.id()); } - let repo = self - .repo_loader - .create_from(data.operation, data.view, data.index); - self.closed = true; - repo + self.repo_loader + .create_from(data.operation, data.view, data.index) } pub fn leave_unpublished(mut self) -> Arc { let data = self.data.take().unwrap(); - let repo = self - .repo_loader - .create_from(data.operation, data.view, data.index); - self.closed = true; - repo - } -} - -impl Drop for UnpublishedOperation { - fn drop(&mut self) { - if !self.closed && !std::thread::panicking() { - eprintln!("BUG: UnpublishedOperation was dropped without being closed."); - } + self.repo_loader + .create_from(data.operation, data.view, data.index) } } From 51f9d6f0440c74985c695b64bc790cd4dae03bb4 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Fri, 1 Mar 2024 12:21:41 -0500 Subject: [PATCH 2/4] Remove the std::Option around UnpublishedOperation::data The Option is unnecessary now since `UnpublishedOperation` doesn't implement the Drop trait (the `MustClose` member implements it instead). --- lib/src/transaction.rs | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 062d0a0c0d..92855c367e 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -162,7 +162,7 @@ struct NewRepoData { #[must_use = "Either publish() or leave_unpublished() must be called to finish the operation."] pub struct UnpublishedOperation { repo_loader: RepoLoader, - data: Option, + data: NewRepoData, } impl UnpublishedOperation { @@ -172,20 +172,22 @@ impl UnpublishedOperation { view: View, index: Box, ) -> Self { - let data = Some(NewRepoData { - operation, - view, - index, - }); - UnpublishedOperation { repo_loader, data } + UnpublishedOperation { + repo_loader, + data: NewRepoData { + operation, + view, + index, + }, + } } pub fn operation(&self) -> &Operation { - &self.data.as_ref().unwrap().operation + &self.data.operation } - pub fn publish(mut self) -> Arc { - let data = self.data.take().unwrap(); + pub fn publish(self) -> Arc { + let data = self.data; { let _lock = self.repo_loader.op_heads_store().lock(); self.repo_loader @@ -196,9 +198,8 @@ impl UnpublishedOperation { .create_from(data.operation, data.view, data.index) } - pub fn leave_unpublished(mut self) -> Arc { - let data = self.data.take().unwrap(); + pub fn leave_unpublished(self) -> Arc { self.repo_loader - .create_from(data.operation, data.view, data.index) + .create_from(self.data.operation, self.data.view, self.data.index) } } From f5a336664dd4055df9111d561fc60a34bb7ffab6 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Fri, 1 Mar 2024 12:28:22 -0500 Subject: [PATCH 3/4] Replace NewRepoData with ReadonlyRepo in the UnpublishedOperation struct `NewRepoData` is just a container that holds data used to construct a `ReadonlyRepo`. The `ReaonlyRepo` is always constructed before the `UnpublishedOperation` is dropped, so we can simply construct the `ReadonlyRepo` upfront and delete the `NewRepoData` type. --- lib/src/transaction.rs | 35 +++++++++-------------------------- 1 file changed, 9 insertions(+), 26 deletions(-) diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index 92855c367e..aace3b55eb 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -147,12 +147,6 @@ pub fn create_op_metadata( } } -struct NewRepoData { - operation: Operation, - view: View, - index: Box, -} - /// An Operation which has been written to the operation store but not /// published. The repo can be loaded at an unpublished Operation, but the /// Operation will not be visible in the op log if the repo is loaded at head. @@ -162,7 +156,7 @@ struct NewRepoData { #[must_use = "Either publish() or leave_unpublished() must be called to finish the operation."] pub struct UnpublishedOperation { repo_loader: RepoLoader, - data: NewRepoData, + repo: Arc, } impl UnpublishedOperation { @@ -172,34 +166,23 @@ impl UnpublishedOperation { view: View, index: Box, ) -> Self { - UnpublishedOperation { - repo_loader, - data: NewRepoData { - operation, - view, - index, - }, - } + let repo = repo_loader.create_from(operation, view, index); + UnpublishedOperation { repo_loader, repo } } pub fn operation(&self) -> &Operation { - &self.data.operation + self.repo.operation() } pub fn publish(self) -> Arc { - let data = self.data; - { - let _lock = self.repo_loader.op_heads_store().lock(); - self.repo_loader - .op_heads_store() - .update_op_heads(data.operation.parent_ids(), data.operation.id()); - } + let _lock = self.repo_loader.op_heads_store().lock(); self.repo_loader - .create_from(data.operation, data.view, data.index) + .op_heads_store() + .update_op_heads(self.operation().parent_ids(), self.operation().id()); + self.repo } pub fn leave_unpublished(self) -> Arc { - self.repo_loader - .create_from(self.data.operation, self.data.view, self.data.index) + self.repo } } From 7f0397bb901f5db4c07cccc3d71510d87e2e56d3 Mon Sep 17 00:00:00 2001 From: Evan Mesterhazy Date: Sat, 2 Mar 2024 14:36:57 -0500 Subject: [PATCH 4/4] Store OpHeadsStore in UnpublishedOperation instead of RepoLoader The only thing we need from the `RepoLoader` is the `OpHeadsStore`, so we can extract it in UnpublishedOperation::new instead of keeping the entire `RepoLoader` around. --- lib/src/transaction.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/lib/src/transaction.rs b/lib/src/transaction.rs index aace3b55eb..a5daebd85a 100644 --- a/lib/src/transaction.rs +++ b/lib/src/transaction.rs @@ -20,6 +20,7 @@ use itertools::Itertools as _; use crate::backend::Timestamp; use crate::index::ReadonlyIndex; +use crate::op_heads_store::OpHeadsStore; use crate::op_store::OperationMetadata; use crate::operation::Operation; use crate::repo::{MutableRepo, ReadonlyRepo, Repo, RepoLoader, RepoLoaderError}; @@ -121,7 +122,7 @@ impl Transaction { .index_store() .write_index(mut_index, operation.id()) .unwrap(); - UnpublishedOperation::new(base_repo.loader(), operation, view, index) + UnpublishedOperation::new(&base_repo.loader(), operation, view, index) } } @@ -155,19 +156,21 @@ pub fn create_op_metadata( /// finish the operation. #[must_use = "Either publish() or leave_unpublished() must be called to finish the operation."] pub struct UnpublishedOperation { - repo_loader: RepoLoader, + op_heads_store: Arc, repo: Arc, } impl UnpublishedOperation { fn new( - repo_loader: RepoLoader, + repo_loader: &RepoLoader, operation: Operation, view: View, index: Box, ) -> Self { - let repo = repo_loader.create_from(operation, view, index); - UnpublishedOperation { repo_loader, repo } + UnpublishedOperation { + op_heads_store: repo_loader.op_heads_store().clone(), + repo: repo_loader.create_from(operation, view, index), + } } pub fn operation(&self) -> &Operation { @@ -175,9 +178,8 @@ impl UnpublishedOperation { } pub fn publish(self) -> Arc { - let _lock = self.repo_loader.op_heads_store().lock(); - self.repo_loader - .op_heads_store() + let _lock = self.op_heads_store.lock(); + self.op_heads_store .update_op_heads(self.operation().parent_ids(), self.operation().id()); self.repo }