Write manifests atomically

In the original manifest updating process, the file was truncated first
before was written with new contents. There is a small chance that
the web server may read back empty file or incomplete file.

The new code makes the update operation atomic (by writing to a temporary
file first then replace the old file with the temporary file).

Fixes #186

Change-Id: I2fd564cb12b922b032c0e9f70d2132a5b12ff098

packager/hls/base/master_playlist.cc

@@ -13,7 +13,6 @@
 #include "packager/base/strings/stringprintf.h"
 #include "packager/hls/base/media_playlist.h"
 #include "packager/media/file/file.h"
-#include "packager/media/file/file_closer.h"
 #include "packager/version/version.h"
 
 namespace shaka {
@@ -162,25 +161,11 @@ bool MasterPlaylist::WriteMasterPlaylist(const std::string& base_url,
       base::FilePath::FromUTF8Unsafe(output_dir)
           .Append(base::FilePath::FromUTF8Unsafe(file_name_))
           .AsUTF8Unsafe();
-  std::unique_ptr<media::File, media::FileCloser> file(
+  if (!media::File::WriteFileAtomically(file_path.c_str(), content)) {
-      media::File::Open(file_path.c_str(), "w"));
+    LOG(ERROR) << "Failed to write master playlist to: " << file_path;
-  if (!file) {
-    LOG(ERROR) << "Failed to open file " << file_path;
-    return false;
-  }
-
-  int64_t bytes_written = file->Write(content.data(), content.size());
-  if (bytes_written < 0) {
-    LOG(ERROR) << "Error while writing master playlist " << file_path;
-    return false;
-  }
-  if (static_cast<size_t>(bytes_written) != content.size()) {
-    LOG(ERROR) << "Written " << bytes_written << " but content size is "
-               << content.size() << " " << file_path;
     return false;
   }
   written_playlist_ = content;
-
   return true;
 }
 

packager/hls/base/master_playlist_unittest.cc

@@ -7,8 +7,7 @@
 #include <gmock/gmock.h>
 #include <gtest/gtest.h>
 
-#include "packager/base/files/file_util.h"
+#include "packager/base/files/file_path.h"
-#include "packager/base/files/scoped_temp_dir.h"
 #include "packager/hls/base/master_playlist.h"
 #include "packager/hls/base/media_playlist.h"
 #include "packager/hls/base/mock_media_playlist.h"
@@ -37,32 +36,21 @@ const MediaPlaylist::MediaPlaylistType kVodPlaylist =
 
 class MasterPlaylistTest : public ::testing::Test {
  protected:
-  MasterPlaylistTest() : master_playlist_(kDefaultMasterPlaylistName) {}
+  MasterPlaylistTest()
+      : master_playlist_(kDefaultMasterPlaylistName),
+        test_output_dir_("memory://test_dir"),
+        master_playlist_path_(
+            FilePath::FromUTF8Unsafe(test_output_dir_)
+                .Append(FilePath::FromUTF8Unsafe(kDefaultMasterPlaylistName))
+                .AsUTF8Unsafe()) {}
 
   void SetUp() override {
     SetPackagerVersionForTesting("test");
-    GetOutputDir(&test_output_dir_path_, &test_output_dir_);
   }
 
   MasterPlaylist master_playlist_;
-  FilePath test_output_dir_path_;
   std::string test_output_dir_;
-
+  std::string master_playlist_path_;
- private:
-  // Creates a path to the output directory for writing out playlists.
-  // |temp_dir_path| is set to the temporary directory so that it can be opened
-  // using base::File* related API.
-  // |output_dir| is set to an equivalent value to |temp_dir_path| but formatted
-  // so that media::File interface can Open it.
-  void GetOutputDir(FilePath* temp_dir_path, std::string* output_dir) {
-    ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
-    ASSERT_TRUE(temp_dir_.IsValid());
-    *temp_dir_path = temp_dir_.path();
-    // TODO(rkuroiwa): Use memory file sys once prefix is exposed.
-    *output_dir = temp_dir_.path().AsUTF8Unsafe() + "/";
-  }
-
-  base::ScopedTempDir temp_dir_;
 };
 
 TEST_F(MasterPlaylistTest, AddMediaPlaylist) {
@@ -88,14 +76,9 @@ TEST_F(MasterPlaylistTest, WriteMasterPlaylistOneVideo) {
   const char kBaseUrl[] = "http://myplaylistdomain.com/";
   EXPECT_TRUE(master_playlist_.WriteMasterPlaylist(kBaseUrl, test_output_dir_));
 
-  FilePath master_playlist_path =
-    test_output_dir_path_.Append(FilePath::FromUTF8Unsafe(
-        kDefaultMasterPlaylistName));
-  ASSERT_TRUE(base::PathExists(master_playlist_path))
-      << "Cannot find " << master_playlist_path.value();
-
   std::string actual;
-  ASSERT_TRUE(base::ReadFileToString(master_playlist_path, &actual));
+  ASSERT_TRUE(
+      media::File::ReadFileToString(master_playlist_path_.c_str(), &actual));
 
   const std::string expected =
       "#EXTM3U\n"
@@ -174,14 +157,9 @@ TEST_F(MasterPlaylistTest, WriteMasterPlaylistVideoAndAudio) {
   const char kBaseUrl[] = "http://playlists.org/";
   EXPECT_TRUE(master_playlist_.WriteMasterPlaylist(kBaseUrl, test_output_dir_));
 
-  FilePath master_playlist_path =
-    test_output_dir_path_.Append(FilePath::FromUTF8Unsafe(
-        kDefaultMasterPlaylistName));
-  ASSERT_TRUE(base::PathExists(master_playlist_path))
-      << "Cannot find " << master_playlist_path.value();
-
   std::string actual;
-  ASSERT_TRUE(base::ReadFileToString(master_playlist_path, &actual));
+  ASSERT_TRUE(
+      media::File::ReadFileToString(master_playlist_path_.c_str(), &actual));
 
   const std::string expected =
       "#EXTM3U\n"
@@ -250,13 +228,9 @@ TEST_F(MasterPlaylistTest, WriteMasterPlaylistMultipleAudioGroups) {
   const char kBaseUrl[] = "http://anydomain.com/";
   EXPECT_TRUE(master_playlist_.WriteMasterPlaylist(kBaseUrl, test_output_dir_));
 
-  FilePath master_playlist_path = test_output_dir_path_.Append(
-      FilePath::FromUTF8Unsafe(kDefaultMasterPlaylistName));
-  ASSERT_TRUE(base::PathExists(master_playlist_path))
-      << "Cannot find " << master_playlist_path.value();
-
   std::string actual;
-  ASSERT_TRUE(base::ReadFileToString(master_playlist_path, &actual));
+  ASSERT_TRUE(
+      media::File::ReadFileToString(master_playlist_path_.c_str(), &actual));
 
   const std::string expected =
       "#EXTM3U\n"

packager/hls/base/media_playlist.cc

@@ -14,7 +14,6 @@
 #include "packager/base/strings/stringprintf.h"
 #include "packager/media/base/language_utils.h"
 #include "packager/media/file/file.h"
-#include "packager/media/file/file_closer.h"
 #include "packager/version/version.h"
 
 namespace shaka {
@@ -333,27 +332,10 @@ bool MediaPlaylist::WriteToFile(const std::string& file_path) {
     content += "#EXT-X-ENDLIST\n";
   }
 
-  std::unique_ptr<media::File, media::FileCloser> file(
+  if (!media::File::WriteFileAtomically(file_path.c_str(), content)) {
-      media::File::Open(file_path.c_str(), "w"));
+    LOG(ERROR) << "Failed to write playlist to: " << file_path;
-  if (!file) {
-    LOG(ERROR) << "Failed to open file " << file_path;
     return false;
   }
-  int64_t bytes_written = file->Write(content.data(), content.size());
-  if (bytes_written < 0) {
-    LOG(ERROR) << "Error while writing playlist to file.";
-    return false;
-  }
-
-  // TODO(rkuroiwa): There are at least 2 while (remaining_bytes > 0) logic in
-  // this library to handle partial writes by File. Dedup them and use it here
-  // has well.
-  if (static_cast<size_t>(bytes_written) < content.size()) {
-    LOG(ERROR) << "Failed to write the whole playlist. Wrote " << bytes_written
-               << " but the playlist is " << content.size() << " bytes.";
-    return false;
-  }
-
   return true;
 }
 

packager/media/file/file.cc

@@ -9,7 +9,9 @@
 #include <gflags/gflags.h>
 #include <algorithm>
 #include <memory>
+#include "packager/base/files/important_file_writer.h"
 #include "packager/base/logging.h"
+#include "packager/base/strings/string_piece.h"
 #include "packager/media/file/local_file.h"
 #include "packager/media/file/memory_file.h"
 #include "packager/media/file/threaded_io_file.h"
@@ -39,12 +41,15 @@ namespace {
 
 typedef File* (*FileFactoryFunction)(const char* file_name, const char* mode);
 typedef bool (*FileDeleteFunction)(const char* file_name);
+typedef bool (*FileAtomicWriteFunction)(const char* file_name,
+                                        const std::string& contents);
 
-struct SupportedTypeInfo {
+struct FileTypeInfo {
   const char* type;
   size_t type_length;
   const FileFactoryFunction factory_function;
   const FileDeleteFunction delete_function;
+  const FileAtomicWriteFunction atomic_write_function;
 };
 
 File* CreateLocalFile(const char* file_name, const char* mode) {
@@ -55,6 +60,12 @@ bool DeleteLocalFile(const char* file_name) {
   return LocalFile::Delete(file_name);
 }
 
+bool WriteLocalFileAtomically(const char* file_name,
+                              const std::string& contents) {
+  return base::ImportantFileWriter::WriteFileAtomically(
+      base::FilePath::FromUTF8Unsafe(file_name), contents);
+}
+
 File* CreateUdpFile(const char* file_name, const char* mode) {
   if (strcmp(mode, "r")) {
     NOTIMPLEMENTED() << "UdpFile only supports read (receive) mode.";
@@ -72,27 +83,43 @@ bool DeleteMemoryFile(const char* file_name) {
   return true;
 }
 
-static const SupportedTypeInfo kSupportedTypeInfo[] = {
+static const FileTypeInfo kFileTypeInfo[] = {
   {
     kLocalFilePrefix,
     strlen(kLocalFilePrefix),
     &CreateLocalFile,
-    &DeleteLocalFile
+    &DeleteLocalFile,
+    &WriteLocalFileAtomically,
   },
   {
     kUdpFilePrefix,
     strlen(kUdpFilePrefix),
     &CreateUdpFile,
-    NULL
+    nullptr,
+    nullptr
   },
   {
     kMemoryFilePrefix,
     strlen(kMemoryFilePrefix),
     &CreateMemoryFile,
-    &DeleteMemoryFile
+    &DeleteMemoryFile,
+    nullptr
   },
 };
 
+const FileTypeInfo* GetFileTypeInfo(base::StringPiece file_name,
+                                    base::StringPiece* real_file_name) {
+  for (const FileTypeInfo& file_type : kFileTypeInfo) {
+    if (strncmp(file_type.type, file_name.data(), file_type.type_length) == 0) {
+      *real_file_name = file_name.substr(file_type.type_length);
+      return &file_type;
+    }
+  }
+  // Otherwise we default to the first file type, which is LocalFile.
+  *real_file_name = file_name;
+  return &kFileTypeInfo[0];
+}
+
 }  // namespace
 
 File* File::Create(const char* file_name, const char* mode) {
@@ -123,19 +150,10 @@ File* File::Create(const char* file_name, const char* mode) {
 }
 
 File* File::CreateInternalFile(const char* file_name, const char* mode) {
-  std::unique_ptr<File, FileCloser> internal_file;
+  base::StringPiece real_file_name;
-  for (size_t i = 0; i < arraysize(kSupportedTypeInfo); ++i) {
+  const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name);
-    const SupportedTypeInfo& type_info = kSupportedTypeInfo[i];
+  DCHECK(file_type);
-    if (strncmp(type_info.type, file_name, type_info.type_length) == 0) {
+  return file_type->factory_function(real_file_name.data(), mode);
-      internal_file.reset(type_info.factory_function(
-          file_name + type_info.type_length, mode));
-    }
-  }
-  // Otherwise we assume it is a local file
-  if (!internal_file)
-    internal_file.reset(CreateLocalFile(file_name, mode));
-
-  return internal_file.release();
 }
 
 File* File::Open(const char* file_name, const char* mode) {
@@ -161,16 +179,12 @@ File* File::OpenWithNoBuffering(const char* file_name, const char* mode) {
 }
 
 bool File::Delete(const char* file_name) {
-  for (size_t i = 0; i < arraysize(kSupportedTypeInfo); ++i) {
+  base::StringPiece real_file_name;
-    const SupportedTypeInfo& type_info = kSupportedTypeInfo[i];
+  const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name);
-    if (strncmp(type_info.type, file_name, type_info.type_length) == 0) {
+  DCHECK(file_type);
-      return type_info.delete_function ?
+  return file_type->delete_function
-          type_info.delete_function(file_name + type_info.type_length) :
+             ? file_type->delete_function(real_file_name.data())
-          false;
+             : false;
-    }
-  }
-  // Otherwise we assume it is a local file
-  return DeleteLocalFile(file_name);
 }
 
 int64_t File::GetFileSize(const char* file_name) {
@@ -200,6 +214,42 @@ bool File::ReadFileToString(const char* file_name, std::string* contents) {
   return len == 0;
 }
 
+bool File::WriteFileAtomically(const char* file_name, const std::string& contents) {
+  base::StringPiece real_file_name;
+  const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name);
+  DCHECK(file_type);
+  if (file_type->atomic_write_function)
+    return file_type->atomic_write_function(real_file_name.data(), contents);
+
+  // Provide a default implementation which may not be atomic unfortunately.
+
+  // Skip the warning message for memory files, which is meant for testing
+  // anyway..
+  if (strncmp(file_name, kMemoryFilePrefix, strlen(kMemoryFilePrefix)) != 0) {
+    LOG(WARNING) << "Writing to " << file_name
+                 << " is not guaranteed to be atomic.";
+  }
+
+  std::unique_ptr<File, FileCloser> file(media::File::Open(file_name, "w"));
+  if (!file) {
+    LOG(ERROR) << "Failed to open file " << file_name;
+    return false;
+  }
+  int64_t bytes_written = file->Write(contents.data(), contents.size());
+  if (bytes_written < 0) {
+    LOG(ERROR) << "Failed to write to file '" << file_name << "' ("
+               << bytes_written << ").";
+    return false;
+  }
+  if (static_cast<size_t>(bytes_written) != contents.size()) {
+    LOG(ERROR) << "Failed to write the whole file to " << file_name
+               << ". Wrote " << bytes_written << " but expecting "
+               << contents.size() << " bytes.";
+    return false;
+  }
+  return true;
+}
+
 bool File::Copy(const char* from_file_name, const char* to_file_name) {
   std::string content;
   if (!ReadFileToString(from_file_name, &content)) {

packager/media/file/file.h

@@ -104,6 +104,13 @@ class File {
   /// @return true on success, false otherwise.
   static bool ReadFileToString(const char* file_name, std::string* contents);
 
+  /// Save `contents` to `file_name` in an atomic manner.
+  /// @param file_name is the destination file name.
+  /// @param contents is the data to be saved.
+  /// @return true on success, false otherwise.
+  static bool WriteFileAtomically(const char* file_name,
+                                  const std::string& contents);
+
   /// Copies files. This is not good for copying huge files. Although not
   /// recommended, it is safe to have source file and destination file name be
   /// the same.

packager/media/file/file_unittest.cc

@@ -159,6 +159,17 @@ TEST_F(LocalFileTest, WriteRead) {
   EXPECT_EQ(data_, read_data);
 }
 
+// There is no easy way to test if a write operation is atomic. This test only
+// ensures the data is written correctly.
+TEST_F(LocalFileTest, AtomicWriteRead) {
+  ASSERT_TRUE(
+      File::WriteFileAtomically(local_file_name_no_prefix_.c_str(), data_));
+  std::string read_data;
+  ASSERT_TRUE(
+      File::ReadFileToString(local_file_name_no_prefix_.c_str(), &read_data));
+  EXPECT_EQ(data_, read_data);
+}
+
 TEST_F(LocalFileTest, WriteFlushCheckSize) {
   const uint32_t kNumCycles(10);
   const uint32_t kNumWrites(10);

packager/mpd/base/mpd_builder.cc

@@ -186,29 +186,6 @@ int SearchTimedOutRepeatIndex(uint64_t timeshift_limit,
   return (timeshift_limit - segment_info.start_time) / segment_info.duration;
 }
 
-// Overload this function to support different types of |output|.
-// Note that this could be done by call MpdBuilder::ToString() and use the
-// result to write to a file, it requires an extra copy.
-bool WriteXmlCharArrayToOutput(xmlChar* doc,
-                               int doc_size,
-                               std::string* output) {
-  DCHECK(doc);
-  DCHECK(output);
-  output->assign(doc, doc + doc_size);
-  return true;
-}
-
-bool WriteXmlCharArrayToOutput(xmlChar* doc,
-                               int doc_size,
-                               media::File* output) {
-  DCHECK(doc);
-  DCHECK(output);
-  if (output->Write(doc, doc_size) < doc_size)
-    return false;
-
-  return output->Flush();
-}
-
 std::string MakePathRelative(const std::string& path,
                              const std::string& mpd_dir) {
   return (path.find(mpd_dir) == 0) ? path.substr(mpd_dir.size()) : path;
@@ -416,17 +393,8 @@ AdaptationSet* MpdBuilder::AddAdaptationSet(const std::string& lang) {
   return adaptation_sets_.back().get();
 }
 
-bool MpdBuilder::WriteMpdToFile(media::File* output_file) {
-  DCHECK(output_file);
-  return WriteMpdToOutput(output_file);
-}
-
 bool MpdBuilder::ToString(std::string* output) {
   DCHECK(output);
-  return WriteMpdToOutput(output);
-}
-template <typename OutputType>
-bool MpdBuilder::WriteMpdToOutput(OutputType* output) {
   static LibXmlInitializer lib_xml_initializer;
 
   xml::scoped_xml_ptr<xmlDoc> doc(GenerateMpd());
@@ -435,16 +403,15 @@ bool MpdBuilder::WriteMpdToOutput(OutputType* output) {
 
   static const int kNiceFormat = 1;
   int doc_str_size = 0;
-  xmlChar* doc_str = NULL;
+  xmlChar* doc_str = nullptr;
   xmlDocDumpFormatMemoryEnc(doc.get(), &doc_str, &doc_str_size, "UTF-8",
                             kNiceFormat);
-
+  output->assign(doc_str, doc_str + doc_str_size);
-  bool result = WriteXmlCharArrayToOutput(doc_str, doc_str_size, output);
   xmlFree(doc_str);
 
   // Cleanup, free the doc.
   doc.reset();
-  return result;
+  return true;
 }
 
 xmlDocPtr MpdBuilder::GenerateMpd() {

packager/mpd/base/mpd_builder.h

@@ -71,15 +71,10 @@ class MpdBuilder {
   /// @return The new adaptation set, which is owned by this instance.
   virtual AdaptationSet* AddAdaptationSet(const std::string& lang);
 
-  /// Write the MPD to specified file.
-  /// @param[out] output_file is MPD destination. output_file will be
-  ///             flushed but not closed.
-  /// @return true on success, false otherwise.
-  bool WriteMpdToFile(media::File* output_file);
-
   /// Writes the MPD to the given string.
   /// @param[out] output is an output string where the MPD gets written.
   /// @return true on success, false otherwise.
+  // TODO(kqyang): Handle file IO in this class as in HLS media_playlist?
   virtual bool ToString(std::string* output);
 
   /// Adjusts the fields of MediaInfo so that paths are relative to the
@@ -103,13 +98,6 @@ class MpdBuilder {
   template <DashProfile profile>
   friend class MpdBuilderTest;
 
-  bool ToStringImpl(std::string* output);
-
-  // This is a helper method for writing out MPDs, called from WriteMpdToFile()
-  // and ToString().
-  template <typename OutputType>
-  bool WriteMpdToOutput(OutputType* output);
-
   // Returns the document pointer to the MPD. This must be freed by the caller
   // using appropriate xmlDocPtr freeing function.
   // On failure, this returns NULL.

packager/mpd/base/mpd_builder_unittest.cc

@@ -9,13 +9,11 @@
 #include <inttypes.h>
 #include <libxml/xmlstring.h>
 
-#include "packager/base/files/file_util.h"
 #include "packager/base/logging.h"
 #include "packager/base/strings/string_number_conversions.h"
 #include "packager/base/strings/string_piece.h"
 #include "packager/base/strings/string_util.h"
 #include "packager/base/strings/stringprintf.h"
-#include "packager/media/file/file.h"
 #include "packager/mpd/base/mpd_builder.h"
 #include "packager/mpd/base/mpd_utils.h"
 #include "packager/mpd/test/mpd_builder_test_helper.h"
@@ -1969,31 +1967,6 @@ TEST_F(OnDemandMpdBuilderTest, MediaInfoMissingBandwidth) {
   ASSERT_FALSE(mpd_.ToString(&mpd_doc));
 }
 
-TEST_F(OnDemandMpdBuilderTest, WriteToFile) {
-  MediaInfo video_media_info = GetTestMediaInfo(kFileNameVideoMediaInfo1);
-  AdaptationSet* video_adaptation_set = mpd_.AddAdaptationSet("");
-  ASSERT_TRUE(video_adaptation_set);
-
-  Representation* video_representation =
-      video_adaptation_set->AddRepresentation(video_media_info);
-  ASSERT_TRUE(video_representation);
-
-  base::FilePath file_path;
-  ASSERT_TRUE(base::CreateTemporaryFile(&file_path));
-  media::File* file = media::File::Open(file_path.AsUTF8Unsafe().c_str(), "w");
-  ASSERT_TRUE(file);
-  ASSERT_TRUE(mpd_.WriteMpdToFile(file));
-  ASSERT_TRUE(file->Close());
-
-  std::string file_content;
-  ASSERT_TRUE(base::ReadFileToString(file_path, &file_content));
-  ASSERT_NO_FATAL_FAILURE(ExpectMpdToEqualExpectedOutputFile(
-      file_content, kFileNameExpectedMpdOutputVideo1));
-
-  const bool kNonRecursive = false;
-  EXPECT_TRUE(DeleteFile(file_path, kNonRecursive));
-}
-
 // Verify that a text path works.
 TEST_F(OnDemandMpdBuilderTest, Text) {
   const char kTextMediaInfo[] =

packager/mpd/base/mpd_notifier_util.cc

@@ -9,14 +9,10 @@
 #include "packager/base/strings/string_number_conversions.h"
 #include "packager/base/strings/string_util.h"
 #include "packager/media/file/file.h"
-#include "packager/media/file/file_closer.h"
 #include "packager/mpd/base/mpd_utils.h"
 
 namespace shaka {
 
-using media::File;
-using media::FileCloser;
-
 bool WriteMpdToFile(const std::string& output_path, MpdBuilder* mpd_builder) {
   CHECK(!output_path.empty());
 
@@ -26,26 +22,11 @@ bool WriteMpdToFile(const std::string& output_path, MpdBuilder* mpd_builder) {
     return false;
   }
 
-  std::unique_ptr<File, FileCloser> file(File::Open(output_path.c_str(), "w"));
+  if (!media::File::WriteFileAtomically(output_path.c_str(), mpd)) {
-  if (!file) {
+    LOG(ERROR) << "Failed to write mpd to: " << output_path;
-    LOG(ERROR) << "Failed to open file for writing: " << output_path;
     return false;
   }
-
+  return true;
-  const char* mpd_char_ptr = mpd.data();
-  size_t mpd_bytes_left = mpd.size();
-  while (mpd_bytes_left > 0) {
-    int64_t length = file->Write(mpd_char_ptr, mpd_bytes_left);
-    if (length <= 0) {
-      LOG(ERROR) << "Failed to write to file '" << output_path << "' ("
-                 << length << ").";
-      return false;
-    }
-    mpd_char_ptr += length;
-    mpd_bytes_left -= length;
-  }
-  // Release the pointer because Close() destructs itself.
-  return file.release()->Close();
 }
 
 ContentType GetContentType(const MediaInfo& media_info) {