Skip to content

Commit

Permalink
Write manifests atomically
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@kqyang
kqyang committed Jul 7, 2017
1 parent 11210b4 commit 2db1867
Show file tree
Hide file tree
Showing 10 changed files with 122 additions and 204 deletions.
19 changes: 2 additions & 17 deletions packager/hls/base/master_playlist.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
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<size_t>(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;
}

Expand Down
56 changes: 15 additions & 41 deletions packager/hls/base/master_playlist_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,7 @@
#include <gmock/gmock.h>
#include <gtest/gtest.h>

#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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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"
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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"
Expand Down
22 changes: 2 additions & 20 deletions packager/hls/base/media_playlist.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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(
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<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;
}

Expand Down
106 changes: 78 additions & 28 deletions packager/media/file/file.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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) {
Expand All @@ -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.";
Expand All @@ -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) {
Expand Down Expand Up @@ -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;
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) {
Expand All @@ -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) {
Expand Down Expand Up @@ -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)) {
Expand Down
7 changes: 7 additions & 0 deletions packager/media/file/file.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
11 changes: 11 additions & 0 deletions packager/media/file/file_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 2db1867

Please sign in to comment.