diff --git a/packager/file/memory_file.cc b/packager/file/memory_file.cc index 60a4a801a4..415a738723 100644 --- a/packager/file/memory_file.cc +++ b/packager/file/memory_file.cc @@ -11,8 +11,10 @@ #include #include #include +#include #include "packager/base/logging.h" +#include "packager/base/synchronization/lock.h" namespace shaka { namespace { @@ -23,35 +25,89 @@ class FileSystem { ~FileSystem() {} static FileSystem* Instance() { - if (!g_file_system_) - g_file_system_.reset(new FileSystem()); - - return g_file_system_.get(); + static FileSystem instance; + return &instance; } - bool Exists(const std::string& file_name) const { - return files_.find(file_name) != files_.end(); + void Delete(const std::string& file_name) { + base::AutoLock auto_lock(lock_); + + if (open_files_.find(file_name) != open_files_.end()) { + LOG(ERROR) << "File '" << file_name + << "' is still open. Deleting an open MemoryFile is not " + "allowed. Exit without deleting the file."; + return; + } + + files_.erase(file_name); } - std::vector* GetFile(const std::string& file_name) { + void DeleteAll() { + base::AutoLock auto_lock(lock_); + if (!open_files_.empty()) { + LOG(ERROR) << "There are still files open. Deleting an open MemoryFile " + "is not allowed. Exit without deleting the file."; + return; + } + files_.clear(); + } + + std::vector* Open(const std::string& file_name, + const std::string& mode) { + base::AutoLock auto_lock(lock_); + + if (open_files_.find(file_name) != open_files_.end()) { + NOTIMPLEMENTED() << "File '" << file_name + << "' is already open. MemoryFile does not support " + "open the same file before it is closed."; + return nullptr; + } + + auto iter = files_.find(file_name); + if (mode == "r") { + if (iter == files_.end()) + return nullptr; + } else if (mode == "w") { + if (iter != files_.end()) + iter->second.clear(); + } else { + NOTIMPLEMENTED() << "File mode '" << mode + << "' not supported by MemoryFile"; + return nullptr; + } + + open_files_[file_name] = mode; return &files_[file_name]; } - void Delete(const std::string& file_name) { files_.erase(file_name); } + bool Close(const std::string& file_name) { + base::AutoLock auto_lock(lock_); - void DeleteAll() { files_.clear(); } + auto iter = open_files_.find(file_name); + if (iter == open_files_.end()) { + LOG(ERROR) << "Cannot close file '" << file_name + << "' which is not open."; + return false; + } + + open_files_.erase(iter); + return true; + } private: - FileSystem() {} + FileSystem(const FileSystem&) = delete; + FileSystem& operator=(const FileSystem&) = delete; - static std::unique_ptr g_file_system_; + FileSystem() = default; - std::map > files_; - DISALLOW_COPY_AND_ASSIGN(FileSystem); + // Filename to file data map. + std::map> files_; + // Filename to file open modes map. + std::map open_files_; + + base::Lock lock_; }; -std::unique_ptr FileSystem::g_file_system_; - } // namespace MemoryFile::MemoryFile(const std::string& file_name, const std::string& mode) @@ -60,6 +116,8 @@ MemoryFile::MemoryFile(const std::string& file_name, const std::string& mode) MemoryFile::~MemoryFile() {} bool MemoryFile::Close() { + if (!FileSystem::Instance()->Close(file_name())) + return false; delete this; return true; } @@ -117,19 +175,10 @@ bool MemoryFile::Tell(uint64_t* position) { } bool MemoryFile::Open() { - FileSystem* file_system = FileSystem::Instance(); - if (mode_ == "r") { - if (!file_system->Exists(file_name())) - return false; - } else if (mode_ == "w") { - file_system->Delete(file_name()); - } else { - NOTIMPLEMENTED() << "File mode " << mode_ << " not supported by MemoryFile"; + file_ = FileSystem::Instance()->Open(file_name(), mode_); + if (!file_) return false; - } - file_ = file_system->GetFile(file_name()); - DCHECK(file_); position_ = 0; return true; } diff --git a/packager/file/memory_file_unittest.cc b/packager/file/memory_file_unittest.cc index bb44a7b5eb..f9813dd352 100644 --- a/packager/file/memory_file_unittest.cc +++ b/packager/file/memory_file_unittest.cc @@ -25,6 +25,7 @@ TEST_F(MemoryFileTest, ModifiesSameFile) { std::unique_ptr writer(File::Open("memory://file1", "w")); ASSERT_TRUE(writer); ASSERT_EQ(kWriteBufferSize, writer->Write(kWriteBuffer, kWriteBufferSize)); + writer.release()->Close(); // Since File::Open should not create a ThreadedIoFile so there should be // no cache. @@ -101,6 +102,7 @@ TEST_F(MemoryFileTest, WriteExistingFileDeletes) { std::unique_ptr file1(File::Open("memory://file1", "w")); ASSERT_TRUE(file1); ASSERT_EQ(kWriteBufferSize, file1->Write(kWriteBuffer, kWriteBufferSize)); + file1.release()->Close(); std::unique_ptr file2(File::Open("memory://file1", "w")); ASSERT_TRUE(file2); diff --git a/packager/packager_test.cc b/packager/packager_test.cc index aa239b88dc..9050388f8d 100644 --- a/packager/packager_test.cc +++ b/packager/packager_test.cc @@ -15,6 +15,7 @@ using testing::MockFunction; using testing::Return; using testing::ReturnArg; using testing::StrEq; +using testing::UnitTest; using testing::WithArgs; namespace shaka { @@ -41,8 +42,6 @@ const double kClearLeadInSeconds = 1.0; class PackagerTest : public ::testing::Test { public: - PackagerTest() {} - void SetUp() override { FILE* f = fopen(kTestFile, "rb"); if (!f) { @@ -50,6 +49,12 @@ class PackagerTest : public ::testing::Test { return; } fclose(f); + + // Use memory file for testing and generate different test directories for + // different tests. + test_directory_ = std::string("memory://test/") + + UnitTest::GetInstance()->current_test_info()->name() + + "/"; } std::string GetFullPath(const std::string& file_name) { @@ -91,8 +96,7 @@ class PackagerTest : public ::testing::Test { } protected: - // Use memory file for testing. - std::string test_directory_ = "memory://test/"; + std::string test_directory_; }; TEST_F(PackagerTest, Version) {