Update "accounts.toml" atomically (#4295) #4308

iequidoo · 2023-04-04T20:13:47Z

No description provided.

link2xt · 2023-04-04T20:17:18Z

src/accounts.rs

@@ -337,9 +337,14 @@ impl Config {

    /// Sync the inmemory representation to disk.
    async fn sync(&self) -> Result<()> {
-        fs::write(&self.file, toml::to_string_pretty(&self.inner)?)
+        let file_tmp = self.file.with_extension("toml.tmp");


Probably better to choose a random name, because if you run sync twice in parallel, one of them may be renaming file when another one has just truncated it.

Also to avoid having lingering temporary files, I think it is fine to create a trash folder right next to the blobdir and garbage-collect it from time to time. Such trash folder is also a common solution for deleting folders atomically, you just move them into trash and then eventually clean it up. We can reuse it in housekeeping by moving unknown dirs out of blobdir and into the trash. Recursive directory deletion will then only happen inside the trash directory and we never have to worry about the directory being deleted only partially.

Note that Android does not have /tmp so we cannot rely on OS to cleanup our temporary directory and have to use our own.

In the current code calling sync() in parallel is only possible when creating multiple Accounts objects which is wrong by design. I think it's better to try keeping this invariant on the type system level. Otherwise if we have multiple Accounts/Config obejcts, there are more complex problems coming like synchronisation of them

link2xt · 2023-04-06T00:36:16Z

src/accounts.rs

@@ -337,9 +337,14 @@ impl Config {

    /// Sync the inmemory representation to disk.
    async fn sync(&self) -> Result<()> {
-        fs::write(&self.file, toml::to_string_pretty(&self.inner)?)
+        let file_tmp = self.file.with_extension("toml.tmp");
+        fs::write(&file_tmp, toml::to_string_pretty(&self.inner)?)


This tokio::fs::write calls std::fs::write inside of spawn_blocking. std::fs::write is only creating the file and writing into it, but does not sync it to disk.

Before rename, sync_data should be called, otherwise it is possible to overwrite old file with a new file which is then not written to the disk.

Yes, and also fsync() called on a directory is needed otherwise rename() durability isn't guaranteed

Looks like opening dirs is only possible in std::fs experimental API: https://doc.rust-lang.org/std/os/wasi/fs/trait.OpenOptionsExt.html#tymethod.directory . And it's a question whether fsyncing for them works (if we talk about POSIX OSes). So, let's just do sync_data() and rename()

Yes, and also fsync() called on a directory is needed otherwise rename() durability isn't guaranteed

It is not a problem, the system may crash after rename and before directory fsync anyway. In this case accounts.toml stays at the old version, but is not corrupted.

Not a big problem, but if we have already reported to a user that we successfully added a new account, but after a hard reset it disappears, it's not very nice. But i don't think it's what can happen frequently, so let's wait until the corresponding Rust API stabilises

Another way of doing things correctly is appending accounts.toml with a new version of config, maybe using the file as a ring buffer, but i think it's not worth the effort

src/accounts.rs

link2xt · 2023-04-06T07:51:07Z

src/accounts.rs

@@ -299,9 +302,11 @@ pub const CONFIG_NAME: &str = "accounts.toml";
 /// Database file name.
 pub const DB_NAME: &str = "dc.db";

+static CONFIG_FILES: Lazy<Mutex<HashSet<PathBuf>>> = Lazy::new(|| Mutex::new(HashSet::new()));


I am not sure what these locks protect from. Someone creating two Accounts objects within the same app is not really an issue. It is much more likely that someone starts two instances of the same bot or client accidentally, and then they are in separate processes anyway.

If you want to protect against multiple Accounts/Config instances operating on the same file, use https://github.com/yoshuawuyts/fd-lock crate, we already depend on it. It is not async, but the whole operation can be run in spawn_blocking which is what tokio::fs does internally anyway. Then it prevents running two bots or two apps (e.g. two Desktops, currently it detects another instance manually, or two KDeltaChats/DeltaTouch which do not protect against this at all).

Of course we must protect from several processes running on the same configuration, but i think better to do that by locking some lockfile, not accounts.toml which can be renamed/removed. I guess there are other reasons to protect from conflicting processes anyway.

As for multiple Accounts objects, at least there was a test doing so (which i'm fixing). And also it's possible if an Accounts object is recreated for some reason. I have no idea why it can be done, but then it's possible to have two objects for some short period. So, i suggest to limit the scope of this PR to the initial problem (non-atomic update of accounts.toml). But maybe a better way to protect from multiple Accounts objects is possible. As for protecting from conflicting processes, i'd suggest to solve that separately.

I summarized this into #4310

Will this CONFIG_FILES be still useful after the introduction of lockfiles?

If an Accounts object will manage the lockfile, looks like CONFIG_FILES may be safely removed, yes

link2xt reviewed Apr 4, 2023

View reviewed changes

link2xt reviewed Apr 6, 2023

View reviewed changes

iequidoo force-pushed the iequidoo/accounts.toml-upd branch from e6857f4 to 3d675d4 Compare April 6, 2023 02:12

iequidoo commented Apr 6, 2023

View reviewed changes

src/accounts.rs Outdated Show resolved Hide resolved

iequidoo force-pushed the iequidoo/accounts.toml-upd branch from 3d675d4 to 39d30d9 Compare April 6, 2023 03:30

iequidoo marked this pull request as ready for review April 6, 2023 03:38

iequidoo requested review from link2xt and r10s April 6, 2023 03:39

link2xt reviewed Apr 6, 2023

View reviewed changes

iequidoo requested a review from link2xt April 6, 2023 16:21

link2xt mentioned this pull request Apr 6, 2023

Add lockfile to account manager and/or contexts #4310

Closed

link2xt approved these changes Apr 6, 2023

View reviewed changes

Update "accounts.toml" atomically (#4295)

1c8ec92

iequidoo force-pushed the iequidoo/accounts.toml-upd branch from 39d30d9 to 1c8ec92 Compare April 7, 2023 01:58

iequidoo merged commit 36bec9c into master Apr 7, 2023

iequidoo deleted the iequidoo/accounts.toml-upd branch April 7, 2023 02:52

link2xt mentioned this pull request Apr 7, 2023

accounts.toml write is not atomic #4295

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update "accounts.toml" atomically (#4295) #4308

Update "accounts.toml" atomically (#4295) #4308

iequidoo commented Apr 4, 2023

link2xt Apr 4, 2023

link2xt Apr 4, 2023 •

edited

Loading

iequidoo Apr 5, 2023

link2xt Apr 6, 2023

iequidoo Apr 6, 2023

iequidoo Apr 6, 2023 •

edited

Loading

link2xt Apr 6, 2023

iequidoo Apr 7, 2023

iequidoo Apr 7, 2023

link2xt Apr 6, 2023 •

edited

Loading

iequidoo Apr 6, 2023

link2xt Apr 6, 2023

iequidoo Apr 7, 2023

Update "accounts.toml" atomically (#4295) #4308

Update "accounts.toml" atomically (#4295) #4308

Conversation

iequidoo commented Apr 4, 2023

Choose a reason for hiding this comment

link2xt Apr 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iequidoo Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

link2xt Apr 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

link2xt Apr 4, 2023 •

edited

Loading

iequidoo Apr 6, 2023 •

edited

Loading

link2xt Apr 6, 2023 •

edited

Loading