[clangd] Change disk-backed storage to be atomic

[kadircet]

Summary

There was a chance that multiple clangd instances could try to write
same shard, in which case we would get a malformed file most likely. This patch
changes the writing mechanism to first write to a temporary file and then rename
it to fit real destination. Which is guaranteed to be atomic by POSIX.

[kadircet]

• Fix returns

[ilya-biryukov]

NIT: I believe we're missing a hyphen in the change description, i.e. it should be disk-backed.

clangd/index/BackgroundIndexStorage.cpp

76

Maybe extract this into a helper function?
Something like

std::error_code writeAtomically(StringRef OutPath, function_ref<void(raw_ostream&)> Writer) {
 /// create temp file, open output stream.
 Writer(OS):
 /// Move the temp file into OutPath.
}

This would keep the storeShard function as readable as it is now

78

Maybe create these temporary files in the same dir? That would ensure the move operations do not require transferring data between physical devices.
If the output file is /some/path/foo.bar, we could create files named /some/path/foo.bar.tmp#####

[kadircet]

• Address comments

[ilya-biryukov]

clangd/index/BackgroundIndexStorage.cpp

42

There's a helper in LLVM that will do this, llvm::createUniqueFile(), I believe it also tries multiple times in case of clashes.

55

We should not remove if no error occurred, as the same name at this point can be taken by a different action.

[kadircet]

• Use creauteUniqueFile.
• Don't remove on success.

[ilya-biryukov]

LGTM with a few nits

clangd/index/BackgroundIndexStorage.cpp

39

NIT: use llvm::function_ref here. It forces to clearly state the signature of the function and allows to avoid templates.

44

NIT: I believe there's an overloaded operator +(StringRef, const char*) that would produce a Twine, so maybe simplify to OutPath + ".tmp.%%%%%"?
Just a suggestion, up to you.

60

NIT: s/it/the name/

[kadircet]

• Address comments

clangd/index/BackgroundIndexStorage.cpp

 //== BackgroundIndexStorage.cpp - Provide caching support to BackgroundIndex ==/
 //
 //                     The LLVM Compiler Infrastructure
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Logger.h"
 #include "index/Background.h"
+#include "llvm/ADT/ScopeExit.h"
+#include "llvm/Support/Error.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/SHA1.h"
 
 namespace clang {
 namespace clangd {
 namespace {
std::string getShardPathFromFilePath(llvm::StringRef ShardRoot,
   llvm::SmallString<128> ShardRootSS(ShardRoot);
   llvm::sys::path::append(ShardRootSS, llvm::sys::path::filename(FilePath) +
                                            "." + llvm::toHex(digest(FilePath)) +
                                            ".idx");
   return ShardRootSS.str();
 }
 
 // Uses disk as a storage for index shards. Creates a directory called
 // ".clangd-index/" under the path provided during construction.
 class DiskBackedIndexStorage : public BackgroundIndexStorage {
   std::string DiskShardRoot;
 
 public:
   // Sets DiskShardRoot to (Directory + ".clangd-index/") which is the base
   // directory for all shard files.
   DiskBackedIndexStorage(llvm::StringRef Directory) {
     llvm::SmallString<128> CDBDirectory(Directory);
     llvm::sys::path::append(CDBDirectory, ".clangd-index/");
     DiskShardRoot = CDBDirectory.str();
     std::error_code OK;
     std::error_code EC = llvm::sys::fs::create_directory(DiskShardRoot);
     if (EC != OK) {
       elog("Failed to create directory {0} for index storage: {1}",
            DiskShardRoot, EC.message());
     }
   }
 
   std::unique_ptr<IndexFileIn>
   loadShard(llvm::StringRef ShardIdentifier) const override {
     const std::string ShardPath =
         getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
     auto Buffer = llvm::MemoryBuffer::getFile(ShardPath);
     if (!Buffer)
       return nullptr;
     if (auto I = readIndexFile(Buffer->get()->getBuffer()))
       return llvm::make_unique<IndexFileIn>(std::move(*I));
     else
       elog("Error while reading shard {0}: {1}", ShardIdentifier,
            I.takeError());
     return nullptr;
   }
 
   llvm::Error storeShard(llvm::StringRef ShardIdentifier,
                          IndexFileOut Shard) const override {
-    auto ShardPath = getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
-    std::error_code EC;
-    llvm::raw_fd_ostream OS(ShardPath, EC);
+    // Write to a temporary file first.
+    llvm::SmallString<64> TempPath;
+    int FD;
+    auto EC = llvm::sys::fs::createTemporaryFile("clangd", "index-shard", FD,
+                                                 TempPath);
     if (EC)
       return llvm::errorCodeToError(EC);
+    // Make sure temp file is destroyed at exit.
+    auto _ =
+        llvm::make_scope_exit([TempPath] { llvm::sys::fs::remove(TempPath); });
+    llvm::raw_fd_ostream OS(FD, true);
     OS << Shard;
     OS.close();
+    if (OS.has_error())
+      return llvm::errorCodeToError(OS.error());
+
+    // Then move to real location.
+    const auto ShardPath =
+        getShardPathFromFilePath(DiskShardRoot, ShardIdentifier);
+    EC = llvm::sys::fs::rename(TempPath, ShardPath);
+    if (EC)
+      return llvm::errorCodeToError(OS.error());
     return llvm::errorCodeToError(OS.error());
   }
 };
 
 // Doesn't persist index shards anywhere (used when the CDB dir is unknown).
 // We could consider indexing into ~/.clangd/ or so instead.
 class NullStorage : public BackgroundIndexStorage {
 public: