From 9c861d03f7f85de8276be7bf8d5fe3d011e68db0 Mon Sep 17 00:00:00 2001 From: KongQun Yang Date: Thu, 7 Sep 2017 16:05:20 -0700 Subject: [PATCH] Report an error when disk is full Previously packaging completes successfully without any error or warning. With the fix, an error will be reported if write fails. It may appear as "Cannot close file error" as we use threaded IO, which could delay the error reporting until Close() call, so the user of the File API needs to make sure Close() returns successfully. Also fixed a deadlock in threaded_io_file if internal_file->Write fails. Fixes #160 Change-Id: I17f945150fb4021d2dcdbe784e557673f53ca583 --- packager/file/file.cc | 12 ++++++++++-- packager/file/local_file.cc | 16 ++++++++++++++-- packager/file/threaded_io_file.cc | 12 ++++++++++-- .../formats/mp4/single_segment_segmenter.cc | 16 ++++++++++++++-- packager/media/formats/webm/mkv_writer.cc | 5 ++++- 5 files changed, 52 insertions(+), 9 deletions(-) diff --git a/packager/file/file.cc b/packager/file/file.cc index 14a2f4f687..6c4933f6db 100644 --- a/packager/file/file.cc +++ b/packager/file/file.cc @@ -250,8 +250,10 @@ bool File::WriteStringToFile(const char* file_name, << contents.size() << " bytes."; return false; } - if (!file->Flush()) { - LOG(ERROR) << "Failed to flush the file '" << file_name << "'."; + if (!file.release()->Close()) { + LOG(ERROR) + << "Failed to close file '" << file_name + << "', possibly file permission issue or running out of disk space."; return false; } return true; @@ -302,6 +304,12 @@ bool File::Copy(const char* from_file_name, const char* to_file_name) { total_bytes_written += bytes_written; } + if (!output_file.release()->Close()) { + LOG(ERROR) + << "Failed to close file '" << to_file_name + << "', possibly file permission issue or running out of disk space."; + return false; + } return true; } diff --git a/packager/file/local_file.cc b/packager/file/local_file.cc index 236f996e61..46c1dfc024 100644 --- a/packager/file/local_file.cc +++ b/packager/file/local_file.cc @@ -37,13 +37,25 @@ bool LocalFile::Close() { int64_t LocalFile::Read(void* buffer, uint64_t length) { DCHECK(buffer != NULL); DCHECK(internal_file_ != NULL); - return fread(buffer, sizeof(char), length, internal_file_); + size_t bytes_read = fread(buffer, sizeof(char), length, internal_file_); + VLOG(2) << "Read " << length << " return " << bytes_read << " error " + << ferror(internal_file_); + if (bytes_read == 0 && ferror(internal_file_) != 0) { + return -1; + } + return bytes_read; } int64_t LocalFile::Write(const void* buffer, uint64_t length) { DCHECK(buffer != NULL); DCHECK(internal_file_ != NULL); - return fwrite(buffer, sizeof(char), length, internal_file_); + size_t bytes_written = fwrite(buffer, sizeof(char), length, internal_file_); + VLOG(2) << "Write " << length << " return " << bytes_written << " error " + << ferror(internal_file_); + if (bytes_written == 0 && ferror(internal_file_) != 0) { + return -1; + } + return bytes_written; } int64_t LocalFile::Size() { diff --git a/packager/file/threaded_io_file.cc b/packager/file/threaded_io_file.cc index 7f668c4916..86547e3cdd 100644 --- a/packager/file/threaded_io_file.cc +++ b/packager/file/threaded_io_file.cc @@ -58,13 +58,14 @@ bool ThreadedIoFile::Open() { bool ThreadedIoFile::Close() { DCHECK(internal_file_); + bool result = true; if (mode_ == kOutputMode) - Flush(); + result = Flush(); cache_.Close(); task_exit_event_.Wait(); - bool result = internal_file_.release()->Close(); + result &= internal_file_.release()->Close(); delete this; return result; } @@ -110,6 +111,9 @@ bool ThreadedIoFile::Flush() { DCHECK(internal_file_); DCHECK_EQ(kOutputMode, mode_); + if (NoBarrier_Load(&internal_file_error_)) + return false; + flushing_ = true; cache_.Close(); flush_complete_event_.Wait(); @@ -204,6 +208,10 @@ void ThreadedIoFile::RunInOutputMode() { if (write_result < 0) { NoBarrier_Store(&internal_file_error_, write_result); cache_.Close(); + if (flushing_) { + flushing_ = false; + flush_complete_event_.Signal(); + } return; } bytes_written += write_result; diff --git a/packager/media/formats/mp4/single_segment_segmenter.cc b/packager/media/formats/mp4/single_segment_segmenter.cc index d4be794201..1433a90310 100644 --- a/packager/media/formats/mp4/single_segment_segmenter.cc +++ b/packager/media/formats/mp4/single_segment_segmenter.cc @@ -90,8 +90,10 @@ Status SingleSegmentSegmenter::DoFinalize() { // Close the temp file to prepare for reading later. if (!temp_file_.release()->Close()) { - return Status(error::FILE_FAILURE, - "Cannot close the temp file " + temp_file_name_); + return Status( + error::FILE_FAILURE, + "Cannot close the temp file " + temp_file_name_ + + ", possibly file permission issue or running out of disk space."); } std::unique_ptr file( @@ -142,6 +144,16 @@ Status SingleSegmentSegmenter::DoFinalize() { UpdateProgress(static_cast(size) / temp_file->Size() * re_segment_progress_target); } + if (!temp_file.release()->Close()) { + return Status(error::FILE_FAILURE, "Cannot close the temp file " + + temp_file_name_ + " after reading."); + } + if (!file.release()->Close()) { + return Status( + error::FILE_FAILURE, + "Cannot close file " + options().output_file_name + + ", possibly file permission issue or running out of disk space."); + } SetComplete(); return Status::OK; } diff --git a/packager/media/formats/webm/mkv_writer.cc b/packager/media/formats/webm/mkv_writer.cc index d77bcf74c5..0d919d0ac6 100644 --- a/packager/media/formats/webm/mkv_writer.cc +++ b/packager/media/formats/webm/mkv_writer.cc @@ -29,7 +29,10 @@ Status MkvWriter::Open(const std::string& name) { Status MkvWriter::Close() { const std::string file_name = file_->file_name(); if (!file_.release()->Close()) { - return Status(error::FILE_FAILURE, "Cannot close file " + file_name); + return Status( + error::FILE_FAILURE, + "Cannot close file " + file_name + + ", possibly file permission issue or running out of disk space."); } return Status::OK; }