fix ~File crash in debug mode

Test Plan: test added

Reviewed By: tjackson@fb.com

FB internal diff: D630163

diff --git a/folly/experimental/File.cpp b/folly/experimental/File.cpp
index 119ae92415ec0cb3c0b62374f62abb1291e4d40b..a0f05f6bb76913bfa18442833bde2749f2a990f0 100644 (file)
--- a/folly/experimental/File.cpp
+++ b/folly/experimental/File.cpp
@@ -37,7 +37,6 @@ void File::close() {
 }
 
 bool File::closeNoThrow() {
-  DCHECK(fd_ != -1);
   int r = ownsFd_ ? ::close(fd_) : 0;
   release();
   return r == 0;

diff --git a/folly/experimental/FileGen-inl.h b/folly/experimental/FileGen-inl.h
index 429cdd7bf9e08a988a0b4ca864bbc77201b4d937..f5050d722496a7bee708d719fca486b709307689 100644 (file)
--- a/folly/experimental/FileGen-inl.h
+++ b/folly/experimental/FileGen-inl.h
@@ -37,8 +37,10 @@ class FileReader : public GenImpl<ByteRange, FileReader> {
   template <class Body>
   bool apply(Body&& body) const {
     for (;;) {
-      ssize_t n = ::read(file_.fd(), buffer_->writableTail(),
-                         buffer_->capacity());
+      ssize_t n;
+      do {
+        n = ::read(file_.fd(), buffer_->writableTail(), buffer_->capacity());
+      } while (n == -1 && errno == EINTR);
       if (n == -1) {
         throw std::system_error(errno, std::system_category(), "read failed");
       }
@@ -91,11 +93,10 @@ class FileWriter : public Operator<FileWriter> {
   void write(ByteRange v) const {
     ssize_t n;
     while (!v.empty()) {
-      n = ::write(file_.fd(), v.data(), v.size());
+      do {
+        n = ::write(file_.fd(), v.data(), v.size());
+      } while (n == -1 && errno == EINTR);
       if (n == -1) {
-        if (errno == EINTR) {
-          continue;
-        }
         throw std::system_error(errno, std::system_category(),
                                 "write() failed");
       }

diff --git a/folly/experimental/test/FileTest.cpp b/folly/experimental/test/FileTest.cpp
new file mode 100644 (file)
index 0000000..6d2dee1
--- /dev/null
+++ b/folly/experimental/test/FileTest.cpp
@@ -0,0 +1,86 @@
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include "folly/experimental/File.h"
+
+#include <glog/logging.h>
+#include <gtest/gtest.h>
+
+#include "folly/Benchmark.h"
+#include "folly/String.h"
+
+using namespace folly;
+
+namespace {
+void expectWouldBlock(ssize_t r) {
+  int savedErrno = errno;
+  EXPECT_EQ(-1, r);
+  EXPECT_EQ(EAGAIN, savedErrno) << errnoStr(errno);
+}
+void expectOK(ssize_t r) {
+  int savedErrno = errno;
+  EXPECT_LE(0, r) << ": errno=" << errnoStr(errno);
+}
+}  // namespace
+
+TEST(File, Simple) {
+  // Open a file, ensure it's indeed open for reading
+  char buf = 'x';
+  {
+    File f("/etc/hosts");
+    EXPECT_NE(-1, f.fd());
+    EXPECT_EQ(1, ::read(f.fd(), &buf, 1));
+    f.close();
+    EXPECT_EQ(-1, f.fd());
+  }
+}
+
+TEST(File, OwnsFd) {
+  // Wrap a file descriptor, make sure that ownsFd works
+  // We'll test that the file descriptor is closed by closing the writing
+  // end of a pipe and making sure that a non-blocking read from the reading
+  // end returns 0.
+
+  char buf = 'x';
+  int p[2];
+  expectOK(::pipe(p));
+  int flags = ::fcntl(p[0], F_GETFL);
+  expectOK(flags);
+  expectOK(::fcntl(p[0], F_SETFL, flags | O_NONBLOCK));
+  expectWouldBlock(::read(p[0], &buf, 1));
+  {
+    File f(p[1]);
+    EXPECT_EQ(p[1], f.fd());
+  }
+  // Ensure that moving the file doesn't close it
+  {
+    File f(p[1]);
+    EXPECT_EQ(p[1], f.fd());
+    File f1(std::move(f));
+    EXPECT_EQ(-1, f.fd());
+    EXPECT_EQ(p[1], f1.fd());
+  }
+  expectWouldBlock(::read(p[0], &buf, 1));  // not closed
+  {
+    File f(p[1], true);
+    EXPECT_EQ(p[1], f.fd());
+  }
+  ssize_t r = ::read(p[0], &buf, 1);  // eof
+  expectOK(r);
+  EXPECT_EQ(0, r);
+  ::close(p[0]);
+}
+