[Work In Progress, early comments welcome] AtomicFile implementation

[vrusinov]

This is the first semi-working implementation of AtomicFile for GothenburgBitFactory/taskwarrior#152. TaskWarrior compiles with this implementation (see atomicfile branch) and all tests pass on my machine. Changes to existing classes are minimal, so most of other libshared users should not be affected. That said, the Directory class was rebased to inherit from Path. This may cause compilation failures in other projects after submodule is updated.

Not ready to be merged yet, but I appreciate any early comments, especially on overall structure.

Few things missing before this can be merged:

• tests, especially failure injection tests. In particular this change should fix TaskWarriror #88 and TaskWarriror #1420, and it should be possible to write a test for it
• Ensure correct file is used in all methods, e.g. in AtomicFile::size
• TaskWarriror directory needs to be fsync()'ed after rename

Follow-up work which I'd prefer to do in next PRs:

• Minor API changes, e.g. it's probably better to rename close to finalize or commit, truncate may be better as overwrite
• Make sure other libshared users still compile
• Look into locking. Existing File::lock() on _original_file should give on-par protection but does not necessarily protects from all races, e.g. during rename. It may be worth locking the whole TaskWarrior directory.
• Cleanup TDB2::commit, e.g. things like _file.append (std::string("")); // Seek to end of file should no longer be necessary
• Performance optimizations: right now TDB2::commit would do AtomicFile::close up to four times sequentially, waiting for each fsync() and rename. We can do better by allowing more writes/fsyncs to go in parallel - via async IO or threads.
• Look into logical correctness of the whole TDB2::commit. Consider what to do when commit of individual file fails.
• Consider making File class private and migrating all users to AtomicFile.

[sruffell]

I've not reviewed the full pull request, but I scanned it to see how it compares with the AtomicFile implementation in timewarrior and I have two general questions.

1. I see that File and AtomicFile now implement the AbstractFile interface. Are you planning to make other changes where users take an AbstractFile and therefore can be passed either a File or AtomicFile depending on a flag at runtime?

2. One of the primary goals of the AtomicFile implementation in timewarrior is to handle updates to several files as a set. Timewarrior needs to update one or more database files, the undo file, and the tags file together as a set. Granted, it is impossible to rename several files atomically as a set, but at least all the temporary files are written before any of them are renamed and the window of time to rename is kept short. Is that something your planning on handling in another way?

Since timewarrior uses libshared, it would seem ideal to me if they both use the same AtomicFile implementation (but it doesn't have to be the one that is already in timewarrior if there are issues with it that make it unsuitable for use in taskwarrior).

[vrusinov]

Thanks, that's kind of feedback/questions/discussions I wanted.

1. "Are you planning to make other changes where users take an AbstractFile and therefore can be passed either a File or AtomicFile depending on a flag at runtime" - not really, and I guess AbstractFile may be unnecessary. It helped me ensure AtomicFile is a drop-in replacement for File, e.g. using it in TaskWarriror is two-line change. I was also expecting AbstractFile won't be pure abstract but turns out there's no functions which implementation can be shared.
   I may keep it during development but clean up later.

2. Yeah, I saw that and it's brilliant. That said, in context of TaskWarrior 'AtomicFile' felt like a wrong place to do handling of multiple different logical files. It feels like it belongs more in database (TDB2) rather than 'File' abstraction. So in order to archive the same I intend to split commit of TDB2 and close of AtomicFile so that writing and finalizing are two different operations. I.e. four sequential commits in TDB2 can be split into four writes and four finalizes. Maybe with "rollback" functionality, sort of the following:

  gather_changes ();
  try {
    pending.write ();
    completed.write ();
    undo.write ();
    backlog.write ();
    pending.commit ();
    completed.commit ();
    undo.commit ();
    backlog.commit ();
  }
  catch (...) {
    pending.rollback ();
    completed.rollback ();
    undo.rollback ();
    backlog.rollback ();
   }

We can also make it a bit faster by making writes & commits parallel / asynchronous (modern storage can do parallel writes and likes deep IO queues).
I'll probably do it only in follow-up PR to minimize the size of each change.

I understand desire to use the same AtomicFile. I looked into timewarrior database implementation (I don't use it so wasn't familiar) and I can see how timewarrior's implementation makes more sense for that use case. But I think it's less convenient for TaskWarriror, especially in context of potential support pluggable support of binary databases like sqlite.

That said, maybe I'm over-complicating things?

[sruffell]

1. "Are you planning to make other changes where users take an AbstractFile and therefore can be passed either a File or AtomicFile depending on a flag at runtime" - not really, and I guess AbstractFile may be unnecessary. It helped me ensure AtomicFile is a drop-in replacement for File, e.g. using it in TaskWarriror is two-line change. I was also expecting AbstractFile won't be pure abstract but turns out there's no functions which implementation can be shared.
   I may keep it during development but clean up later.

👍

... in context of TaskWarrior 'AtomicFile' felt like a wrong place to do handling of multiple different logical files. It feels like it belongs more in database (TDB2) rather than 'File' abstraction.

I don't disagree here. Originally, the AtomicFile class was just meant to be a drop-in replacement for File, but ultimately I had envisioned that the set of files that need to be manipulated together would need to be explicitly specified by the user with an AtomicFileSet. So for example, instead of just passing the path to the AtomicFile constructor, the AtomicFiles objects would need to be created by using a factory function in the set that the file is a part of.

So in order to archive the same I intend to split commit of TDB2 and close of AtomicFile so that writing and finalizing are two different operations. I.e. four sequential commits in TDB2 can be split into four writes and four finalizes. Maybe with "rollback" functionality, sort of the following:

  gather_changes ();
  try {
    pending.write ();
    completed.write ();
    undo.write ();
    backlog.write ();
    pending.commit ();
    completed.commit ();
    undo.commit ();
    backlog.commit ();
  }
  catch (...) {
    pending.rollback ();
    completed.rollback ();
    undo.rollback ();
    backlog.rollback ();
   }

This makes sense to me. Although are you open to something like:

AtomicFileSet fileSet;
pending = fileSet.open(...);
completed = fileSet.open(...);
undo = fileSet.open(...);
backlog = fileSet.open(...);

// ...

gather_changes ();
try {
  pending.write ();
  completed.write ();
  undo.write ();
  backlog.write ();

  fileSet.commit ();
}
catch (...)
{
   // rollback handled by AtomicFileSet destructor if commit was not called.
}

We can also make it a bit faster by making writes & commits parallel / asynchronous (modern storage can do parallel writes and likes deep IO queues).

👍

I'll probably do it only in follow-up PR to minimize the size of each change.

I understand desire to use the same AtomicFile. I looked into timewarrior database implementation (I don't use it so wasn't familiar) and I can see how timewarrior's implementation makes more sense for that use case. But I think it's less convenient for TaskWarriror, especially in context of potential support pluggable support of binary databases like sqlite.

I primarily use timewarrior myself, so am in the same boat as you, but just from the other side.

Pluggable backends is something that timewarrior has a desire for as well. Although, for the most part, my thoughts are that AtomicFile would be used by the backend, and not as part of the interface. I.e., when someone calls database.commit (); they do not care if AtomicFileSet::commit() is used or the commit from the embedded database that is used to implement the Database interface. Those are my thoughts anyway.

That said, maybe I'm over-complicating things?

I don't think so, and I still believe we can come up with an interface that will be usable with both projects if you want to collaborate.

[sruffell]

@vrusinov Just checking if you're waiting on me or not? Would you like me to try and put together a candidate PR?

[vrusinov]

Not waiting, and thanks for feedback!

Busy, and procrastinating a bit, but hope to get back to this soon.