[mesos] branch master updated: Correctly propagated `close` failures in some instances.

[bb...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

bbannier pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git


The following commit(s) were added to refs/heads/master by this push:
     new d838f29  Correctly propagated `close` failures in some instances.
d838f29 is described below

commit d838f2958e18c1a75594ca4f10df132670fcd11e
Author: Benjamin Bannier <be...@mesosphere.io>
AuthorDate: Thu Jan 24 10:44:49 2019 +0100

    Correctly propagated `close` failures in some instances.
    
    When e.g., writing to disk, errors from write might only manifest at
    ::close time. We update some instances to correctly propagate these
    errors instead of dropping them silently. We only propagate the
    ::close error if the write operation succeeded, otherwise we just
    propagate the error from the write operation.
    
    Review: https://reviews.apache.org/r/69082/
---
 3rdparty/stout/include/stout/os/posix/mktemp.hpp | 26 ++++++++--------
 3rdparty/stout/include/stout/os/write.hpp        | 19 +++++++-----
 3rdparty/stout/include/stout/protobuf.hpp        | 38 ++++++++++++++----------
 3 files changed, 47 insertions(+), 36 deletions(-)

diff --git a/3rdparty/stout/include/stout/os/posix/mktemp.hpp b/3rdparty/stout/include/stout/os/posix/mktemp.hpp
index 63b3d1a..8dab259 100644
--- a/3rdparty/stout/include/stout/os/posix/mktemp.hpp
+++ b/3rdparty/stout/include/stout/os/posix/mktemp.hpp
@@ -36,23 +36,25 @@ namespace os {
 inline Try<std::string> mktemp(
     const std::string& path = path::join(os::temp(), "XXXXXX"))
 {
-  char* temp = new char[path.size() + 1];
-  ::memcpy(temp, path.c_str(), path.size() + 1);
+  char* _path = new char[path.size() + 1];
+  ::memcpy(_path, path.c_str(), path.size() + 1);
+
+  int_fd fd = ::mkstemp(_path);
+  Try<std::string> temp = std::string(_path);
+  delete[] _path;
 
-  int_fd fd = ::mkstemp(temp);
   if (fd < 0) {
-    delete[] temp;
-    return ErrnoError();
+    temp = ErrnoError();
   }
 
-  // We ignore the return value of close(). This is because users
-  // calling this function are interested in the return value of
-  // mkstemp(). Also an unsuccessful close() doesn't affect the file.
-  os::close(fd);
+  Try<Nothing> close = os::close(fd);
+
+  // We propagate `close` failures if creation of the temp file was successful.
+  if (temp.isSome() && close.isError()) {
+    return Error("Failed to close '" + stringify(fd) + "':" + close.error());
+  }
 
-  std::string result(temp);
-  delete[] temp;
-  return result;
+  return temp;
 }
 
 } // namespace os {
diff --git a/3rdparty/stout/include/stout/os/write.hpp b/3rdparty/stout/include/stout/os/write.hpp
index f7538f9..89ca7b2 100644
--- a/3rdparty/stout/include/stout/os/write.hpp
+++ b/3rdparty/stout/include/stout/os/write.hpp
@@ -126,21 +126,24 @@ inline Try<Nothing> write(
     return Error(fd.error());
   }
 
-  Try<Nothing> result = write(fd.get(), message);
+  Try<Nothing> write = os::write(fd.get(), message);
 
-  if (sync && result.isSome()) {
+  if (sync && write.isSome()) {
     // We call `fsync()` before closing the file instead of opening it with the
     // `O_SYNC` flag for better performance. See:
     // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
-    result = os::fsync(fd.get());
+    write = os::fsync(fd.get());
   }
 
-  // We ignore the return value of `close()` because users calling this function
-  // are interested in the return value of `write()`, or `fsync()` if `sync` is
-  // set to true. Also an unsuccessful `close()` doesn't affect the write.
-  os::close(fd.get());
+  Try<Nothing> close = os::close(fd.get());
 
-  return result;
+  // We propagate `close` failures if `write` on the file was successful.
+  if (write.isSome() && close.isError()) {
+    write =
+      Error("Failed to close '" + stringify(fd.get()) + "':" + close.error());
+  }
+
+  return write;
 }
 
 
diff --git a/3rdparty/stout/include/stout/protobuf.hpp b/3rdparty/stout/include/stout/protobuf.hpp
index eb4adef..4b3db7e 100644
--- a/3rdparty/stout/include/stout/protobuf.hpp
+++ b/3rdparty/stout/include/stout/protobuf.hpp
@@ -147,21 +147,24 @@ Try<Nothing> write(const std::string& path, const T& t, bool sync = false)
     return Error("Failed to open file '" + path + "': " + fd.error());
   }
 
-  Try<Nothing> result = write(fd.get(), t);
+  Try<Nothing> write = ::protobuf::write(fd.get(), t);
 
-  if (sync && result.isSome()) {
+  if (sync && write.isSome()) {
     // We call `fsync()` before closing the file instead of opening it with the
     // `O_SYNC` flag for better performance. See:
     // http://lkml.iu.edu/hypermail/linux/kernel/0105.3/0353.html
-    result = os::fsync(fd.get());
+    write = os::fsync(fd.get());
   }
 
-  // We ignore the return value of `close()` because users calling this function
-  // are interested in the return value of `write()`, or `fsync()` if `sync` is
-  // set to true. Also an unsuccessful `close()` doesn't affect the write.
-  os::close(fd.get());
+  Try<Nothing> close = os::close(fd.get());
 
-  return result;
+  // We propagate `close` failures if `write` on the file was successful.
+  if (write.isSome() && close.isError()) {
+    return Error(
+        "Failed to close '" + stringify(fd.get()) + "':" + close.error());
+  }
+
+  return write;
 }
 
 
@@ -181,20 +184,23 @@ inline Try<Nothing> append(
     return Error("Failed to open file '" + path + "': " + fd.error());
   }
 
-  Try<Nothing> result = write(fd.get(), message);
+  Try<Nothing> write = protobuf::write(fd.get(), message);
 
-  if (sync && result.isSome()) {
+  if (sync && write.isSome()) {
     // We call `fsync()` before closing the file instead of opening it with the
     // `O_SYNC` flag for better performance.
-    result = os::fsync(fd.get());
+    write = os::fsync(fd.get());
   }
 
-  // NOTE: We ignore the return value of close(). This is because
-  // users calling this function are interested in the return value of
-  // write(). Also an unsuccessful close() doesn't affect the write.
-  os::close(fd.get());
+  Try<Nothing> close = os::close(fd.get());
 
-  return result;
+  // We propagate `close` failures if `write` on the file was successful.
+  if (write.isSome() && close.isError()) {
+    return Error(
+        "Failed to close '" + stringify(fd.get()) + "':" + close.error());
+  }
+
+  return write;
 }