Address more review feedback.

Author: anjenner

llvm/include/llvm/Support/Caching.h

@@ -24,16 +24,32 @@ class MemoryBuffer;
 /// This class wraps an output stream for a file. Most clients should just be
 /// able to return an instance of this base class from the stream callback, but
 /// if a client needs to perform some action after the stream is written to,
-/// that can be done by deriving from this class and overriding the destructor.
+/// that can be done by deriving from this class and overriding the destructor
+/// or the commit() method.
 class CachedFileStream {
 public:
   CachedFileStream(std::unique_ptr<raw_pwrite_stream> OS,
                    std::string OSPath = "")
       : OS(std::move(OS)), ObjectPathName(OSPath) {}
-  virtual Error commit() { return Error::success(); }
+
+  /// Must be called exactly once after the writes to OS have been completed
+  /// but before the CachedFileStream object is destroyed.
+  virtual Error commit() {
+    if (Committed)
+      return createStringError(make_error_code(std::errc::invalid_argument),
+                               Twine("CacheStream already committed."));
+    Committed = true;
+
+    return Error::success();
+  }
+
+  bool Committed = false;
   std::unique_ptr<raw_pwrite_stream> OS;
   std::string ObjectPathName;
-  virtual ~CachedFileStream() = default;
+  virtual ~CachedFileStream() {
+    if (!Committed)
+      report_fatal_error("CachedFileStream was not committed.\n");
+  }
 };
 
 /// This type defines the callback to add a file that is generated on the fly.

llvm/lib/Debuginfod/Debuginfod.cpp

@@ -188,7 +188,11 @@ class StreamedHTTPResponseHandler : public HTTPResponseHandler {
 public:
   StreamedHTTPResponseHandler(CreateStreamFn CreateStream, HTTPClient &Client)
       : CreateStream(CreateStream), Client(Client) {}
+
+  /// Must be called exactly once after the writes have been completed
+  /// but before the StreamedHTTPResponseHandler object is destroyed.
   Error commit();
+
   virtual ~StreamedHTTPResponseHandler() = default;
 
   Error handleBodyChunk(StringRef BodyChunk) override;

llvm/lib/Support/Caching.cpp

@@ -80,7 +80,6 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
       sys::fs::TempFile TempFile;
       std::string ModuleName;
       unsigned Task;
-      bool Committed = false;
 
       CacheStream(std::unique_ptr<raw_pwrite_stream> OS, AddBufferFn AddBuffer,
                   sys::fs::TempFile TempFile, std::string EntryPath,
@@ -90,10 +89,9 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
             ModuleName(ModuleName), Task(Task) {}
 
       Error commit() override {
-        if (Committed)
-          return createStringError(make_error_code(std::errc::invalid_argument),
-                                   Twine("CacheStream already committed."));
-        Committed = true;
+        Error E = CachedFileStream::commit();
+        if (E)
+          return E;
 
         // Make sure the stream is closed before committing it.
         OS.reset();
@@ -119,7 +117,7 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         // AddBuffer a copy of the bytes we wrote in that case. We do this
         // instead of just using the existing file, because the pruner might
         // delete the file before we get a chance to use it.
-        Error E = TempFile.keep(ObjectPathName);
+        E = TempFile.keep(ObjectPathName);
         E = handleErrors(std::move(E), [&](const ECError &E) -> Error {
           std::error_code EC = E.convertToErrorCode();
           if (EC != errc::permission_denied)
@@ -144,11 +142,6 @@ Expected<FileCache> llvm::localCache(const Twine &CacheNameRef,
         AddBuffer(Task, ModuleName, std::move(*MBOrErr));
         return Error::success();
       }
-
-      ~CacheStream() {
-        if (!Committed)
-          report_fatal_error("CacheStream was not committed.\n");
-      }
     };
 
     return [=](size_t Task, const Twine &ModuleName)

llvm/unittests/Support/Caching.cpp

@@ -115,3 +115,49 @@ TEST(Caching, WriteAfterCommit) {
 
   ASSERT_NO_ERROR(sys::fs::remove_directories(CacheDir.str()));
 }
+
+TEST(Caching, NoCommit) {
+  SmallString<256> TempDir;
+  sys::path::system_temp_directory(true, TempDir);
+  SmallString<256> CacheDir;
+  sys::path::append(CacheDir, TempDir, "llvm_test_cache");
+
+  sys::fs::remove_directories(CacheDir.str());
+
+  std::unique_ptr<MemoryBuffer> CachedBuffer;
+  auto AddBuffer = [&CachedBuffer](unsigned Task, const Twine &ModuleName,
+                                   std::unique_ptr<MemoryBuffer> M) {
+    CachedBuffer = std::move(M);
+  };
+  auto CacheOrErr =
+      localCache("LLVMTestCache", "LLVMTest", CacheDir, AddBuffer);
+  ASSERT_TRUE(bool(CacheOrErr));
+
+  FileCache &Cache = *CacheOrErr;
+
+  auto AddStreamOrErr = Cache(1, "foo", "");
+  ASSERT_TRUE(bool(AddStreamOrErr));
+
+  AddStreamFn &AddStream = *AddStreamOrErr;
+  ASSERT_TRUE(AddStream);
+
+  auto FileOrErr = AddStream(1, "");
+  ASSERT_TRUE(bool(FileOrErr));
+
+  CachedFileStream *CFS = FileOrErr->get();
+  (*CFS->OS).write(data, sizeof(data));
+  ASSERT_THAT_ERROR(CFS->commit(), Succeeded());
+
+  EXPECT_DEATH(
+      {
+        auto FileOrErr = AddStream(1, "");
+        ASSERT_TRUE(bool(FileOrErr));
+
+        CachedFileStream *CFS = FileOrErr->get();
+        (*CFS->OS).write(data, sizeof(data));
+      },
+      "")
+      << "destruction without commit did not cause error";
+
+  ASSERT_NO_ERROR(sys::fs::remove_directories(CacheDir.str()));
+}