From 2db1867951806fb281d182c8a22a17c96495c519 Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Thu, 15 Jun 2017 13:00:28 -0700 Subject: [PATCH] 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 | 19 +--- packager/hls/base/master_playlist_unittest.cc | 56 +++------ packager/hls/base/media_playlist.cc | 22 +--- packager/media/file/file.cc | 106 +++++++++++++----- packager/media/file/file.h | 7 ++ packager/media/file/file_unittest.cc | 11 ++ packager/mpd/base/mpd_builder.cc | 39 +------ packager/mpd/base/mpd_builder.h | 14 +-- packager/mpd/base/mpd_builder_unittest.cc | 27 ----- packager/mpd/base/mpd_notifier_util.cc | 25 +---- 10 files changed, 122 insertions(+), 204 deletions(-) diff --git a/packager/hls/base/master_playlist.cc b/packager/hls/base/master_playlist.cc index 6b0f4d2b2b..4751138222 100644 --- a/packager/hls/base/master_playlist.cc +++ b/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 file( - media::File::Open(file_path.c_str(), "w")); - 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(bytes_written) != content.size()) { - LOG(ERROR) << "Written " << bytes_written << " but content size is " - << content.size() << " " << file_path; + if (!media::File::WriteFileAtomically(file_path.c_str(), content)) { + LOG(ERROR) << "Failed to write master playlist to: " << file_path; return false; } written_playlist_ = content; - return true; } diff --git a/packager/hls/base/master_playlist_unittest.cc b/packager/hls/base/master_playlist_unittest.cc index f82b2dae8c..cf0fd8320c 100644 --- a/packager/hls/base/master_playlist_unittest.cc +++ b/packager/hls/base/master_playlist_unittest.cc @@ -7,8 +7,7 @@ #include #include -#include "packager/base/files/file_util.h" -#include "packager/base/files/scoped_temp_dir.h" +#include "packager/base/files/file_path.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_; - - 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_; + std::string master_playlist_path_; }; 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" diff --git a/packager/hls/base/media_playlist.cc b/packager/hls/base/media_playlist.cc index d91e15acb4..135731873d 100644 --- a/packager/hls/base/media_playlist.cc +++ b/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 file( - media::File::Open(file_path.c_str(), "w")); - if (!file) { - LOG(ERROR) << "Failed to open file " << file_path; + if (!media::File::WriteFileAtomically(file_path.c_str(), content)) { + LOG(ERROR) << "Failed to write playlist to: " << 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(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; } diff --git a/packager/media/file/file.cc b/packager/media/file/file.cc index b2a8ba2739..d81adece93 100644 --- a/packager/media/file/file.cc +++ b/packager/media/file/file.cc @@ -9,7 +9,9 @@ #include #include #include +#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 internal_file; - for (size_t i = 0; i < arraysize(kSupportedTypeInfo); ++i) { - const SupportedTypeInfo& type_info = kSupportedTypeInfo[i]; - if (strncmp(type_info.type, file_name, type_info.type_length) == 0) { - 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(); + base::StringPiece real_file_name; + const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name); + DCHECK(file_type); + return file_type->factory_function(real_file_name.data(), mode); } 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) { - const SupportedTypeInfo& type_info = kSupportedTypeInfo[i]; - if (strncmp(type_info.type, file_name, type_info.type_length) == 0) { - return type_info.delete_function ? - type_info.delete_function(file_name + type_info.type_length) : - false; - } - } - // Otherwise we assume it is a local file - return DeleteLocalFile(file_name); + base::StringPiece real_file_name; + const FileTypeInfo* file_type = GetFileTypeInfo(file_name, &real_file_name); + DCHECK(file_type); + return file_type->delete_function + ? file_type->delete_function(real_file_name.data()) + : false; } 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(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(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)) { diff --git a/packager/media/file/file.h b/packager/media/file/file.h index ef66ed663b..67030157c4 100644 --- a/packager/media/file/file.h +++ b/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. diff --git a/packager/media/file/file_unittest.cc b/packager/media/file/file_unittest.cc index 27c0a10dee..24d7a5fe21 100644 --- a/packager/media/file/file_unittest.cc +++ b/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); diff --git a/packager/mpd/base/mpd_builder.cc b/packager/mpd/base/mpd_builder.cc index 072b7c025e..9fc86e3e9d 100644 --- a/packager/mpd/base/mpd_builder.cc +++ b/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 -bool MpdBuilder::WriteMpdToOutput(OutputType* output) { static LibXmlInitializer lib_xml_initializer; xml::scoped_xml_ptr 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); - - bool result = WriteXmlCharArrayToOutput(doc_str, doc_str_size, output); + output->assign(doc_str, doc_str + doc_str_size); xmlFree(doc_str); // Cleanup, free the doc. doc.reset(); - return result; + return true; } xmlDocPtr MpdBuilder::GenerateMpd() { diff --git a/packager/mpd/base/mpd_builder.h b/packager/mpd/base/mpd_builder.h index 7a443658fb..1665309f4c 100644 --- a/packager/mpd/base/mpd_builder.h +++ b/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 friend class MpdBuilderTest; - bool ToStringImpl(std::string* output); - - // This is a helper method for writing out MPDs, called from WriteMpdToFile() - // and ToString(). - template - 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. diff --git a/packager/mpd/base/mpd_builder_unittest.cc b/packager/mpd/base/mpd_builder_unittest.cc index c6c18d7be5..ebd3c7a013 100644 --- a/packager/mpd/base/mpd_builder_unittest.cc +++ b/packager/mpd/base/mpd_builder_unittest.cc @@ -9,13 +9,11 @@ #include #include -#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[] = diff --git a/packager/mpd/base/mpd_notifier_util.cc b/packager/mpd/base/mpd_notifier_util.cc index 96ff81b6ac..e07e9aa380 100644 --- a/packager/mpd/base/mpd_notifier_util.cc +++ b/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(File::Open(output_path.c_str(), "w")); - if (!file) { - LOG(ERROR) << "Failed to open file for writing: " << output_path; + if (!media::File::WriteFileAtomically(output_path.c_str(), mpd)) { + LOG(ERROR) << "Failed to write mpd to: " << output_path; return false; } - - 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(); + return true; } ContentType GetContentType(const MediaInfo& media_info) {