experiments/STT-tensorflow
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

GCS File system: Remove tempfile in destructor regardless of status.

Browse Source 
Otherwise this implementation leaves tempfiles around on failures, filling
up the temp filesystem: the Sync() call fails and Close() returns before the tempfile can be removed on destruction.

By removing the tempfile in the destructor, this mirrors the creation of the
tempfile in the constructor, and ensures it only happens once.

PiperOrigin-RevId: 316171772
Change-Id: I7a6ab9c417cd32b0e1fb46499b80f23bb476b6b6
This commit is contained in:
Vijay Vasudevan 2020-06-12 13:57:03 -07:00 committed by TensorFlower Gardener
parent 05db3739aa
commit 9130ba73e6
2 changed files with 38 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

13
tensorflow/core/platform/cloud/gcs_file_system.cc
View File

@ -416,7 +416,10 @@ class GcsWritableFile : public WritableFile {
std::ofstream::binary | std::ofstream::app);
}
~GcsWritableFile() override { Close().IgnoreError(); }
~GcsWritableFile() override {
Close().IgnoreError();
std::remove(tmp_content_filename_.c_str());
}
Status Append(StringPiece data) override {
TF_RETURN_IF_ERROR(CheckWritable());
@ -431,9 +434,11 @@ class GcsWritableFile : public WritableFile {
Status Close() override {
if (outfile_.is_open()) {
TF_RETURN_IF_ERROR(Sync());
outfile_.close();
std::remove(tmp_content_filename_.c_str());
Status sync_status = Sync();
if (sync_status.ok()) {
outfile_.close();
}
return sync_status;
}
return Status::OK();
}

43
tensorflow/core/platform/cloud/gcs_file_system_test.cc
View File

@ -1225,6 +1225,11 @@ TEST(GcsFileSystemTest, NewWritableFile_ResumeUploadAllAttemptsFail) {
}
TEST(GcsFileSystemTest, NewWritableFile_UploadReturns410) {
std::vector<string> results;
TF_EXPECT_OK(
Env::Default()->GetMatchingPaths("/tmp/tmp_file_tensorflow*", &results));
const int64 tmp_files_before = results.size();
std::vector<HttpRequest*> requests(
{new FakeHttpRequest(
"Uri: https://www.googleapis.com/upload/storage/v1/b/bucket/o?"
@ -1268,21 +1273,31 @@ TEST(GcsFileSystemTest, NewWritableFile_UploadReturns410) {
kTestTimeoutConfig, *kAllowedLocationsDefault,
nullptr /* gcs additional header */);
std::unique_ptr<WritableFile> file;
TF_EXPECT_OK(fs.NewWritableFile("gs://bucket/path/writeable.txt", &file));
{
std::unique_ptr<WritableFile> file;
TF_EXPECT_OK(fs.NewWritableFile("gs://bucket/path/writeable.txt", &file));
TF_EXPECT_OK(file->Append("content1,"));
TF_EXPECT_OK(file->Append("content2"));
const auto& status = file->Close();
EXPECT_EQ(errors::Code::UNAVAILABLE, status.code());
EXPECT_TRUE(
absl::StrContains(status.error_message(),
"Upload to gs://bucket/path/writeable.txt failed, "
"caused by: Not found: important HTTP error 410"))
<< status;
EXPECT_TRUE(absl::StrContains(
status.error_message(), "when uploading gs://bucket/path/writeable.txt"))
<< status;
TF_EXPECT_OK(file->Append("content1,"));
TF_EXPECT_OK(file->Append("content2"));
const auto& status = file->Close();
EXPECT_EQ(errors::Code::UNAVAILABLE, status.code());
EXPECT_TRUE(
absl::StrContains(status.error_message(),
"Upload to gs://bucket/path/writeable.txt failed, "
"caused by: Not found: important HTTP error 410"))
<< status;
EXPECT_TRUE(
absl::StrContains(status.error_message(),
"when uploading gs://bucket/path/writeable.txt"))
<< status;
}
// Check that no new tempfiles were left over after failure and destruction
// of the file.
results.clear();
TF_EXPECT_OK(
Env::Default()->GetMatchingPaths("/tmp/tmp_file_tensorflow*", &results));
EXPECT_EQ(tmp_files_before, results.size());
}
TEST(GcsFileSystemTest, NewWritableFile_NoObjectName) {