From d1028d196481191a6e3adf1b081f736b89c52cfa Mon Sep 17 00:00:00 2001 From: Tudor Bosman Date: Fri, 26 Apr 2013 15:32:36 -0700 Subject: [PATCH] flock locks in folly::File, FileUtil, Exception.h fixes and tests Summary: Test required a separate process, as fcntl locks are always re-granted to a process that holds the lock. Test Plan: new tests, all tests under folly Reviewed By: lucian@fb.com FB internal diff: D791370 --- folly/Exception.h | 66 +++++++++--- folly/File.cpp | 46 ++++++--- folly/File.h | 21 ++++ folly/FileUtil.cpp | 165 ++++++++++++++++++++++++++++++ folly/FileUtil.h | 80 +++++++++++++++ folly/test/ExceptionTest.cpp | 97 ++++++++++++++++++ folly/test/FileTest.cpp | 82 +++++++++++++++ folly/test/FileTestLockHelper.cpp | 39 +++++++ folly/test/SubprocessTest.cpp | 2 +- 9 files changed, 568 insertions(+), 30 deletions(-) create mode 100644 folly/FileUtil.cpp create mode 100644 folly/FileUtil.h create mode 100644 folly/test/ExceptionTest.cpp create mode 100644 folly/test/FileTestLockHelper.cpp diff --git a/folly/Exception.h b/folly/Exception.h index a9300ce4..fad0b691 100644 --- a/folly/Exception.h +++ b/folly/Exception.h @@ -19,61 +19,93 @@ #include +#include #include #include #include "folly/Conv.h" +#include "folly/FBString.h" #include "folly/Likely.h" #include "folly/Portability.h" namespace folly { +// Various helpers to throw appropriate std::system_error exceptions from C +// library errors (returned in errno, as positive return values (many POSIX +// functions), or as negative return values (Linux syscalls)) +// +// The *Explicit functions take an explicit value for errno. + // Helper to throw std::system_error -void throwSystemError(int err, const char* msg) FOLLY_NORETURN; -inline void throwSystemError(int err, const char* msg) { +void throwSystemErrorExplicit(int err, const char*) FOLLY_NORETURN; +inline void throwSystemErrorExplicit(int err, const char* msg) { throw std::system_error(err, std::system_category(), msg); } -// Helper to throw std::system_error from errno -void throwSystemError(const char* msg) FOLLY_NORETURN; -inline void throwSystemError(const char* msg) { - throwSystemError(errno, msg); +template +void throwSystemErrorExplicit(int, Args&&... args) FOLLY_NORETURN; +template +void throwSystemErrorExplicit(int err, Args&&... args) { + throwSystemErrorExplicit( + err, to(std::forward(args)...).c_str()); } // Helper to throw std::system_error from errno and components of a string template -void throwSystemError(Args... args) FOLLY_NORETURN; +void throwSystemError(Args&&... args) FOLLY_NORETURN; template -inline void throwSystemError(Args... args) { - throwSystemError(errno, folly::to(args...)); +void throwSystemError(Args&&... args) { + throwSystemErrorExplicit(errno, std::forward(args)...); } // Check a Posix return code (0 on success, error number on error), throw // on error. -inline void checkPosixError(int err, const char* msg) { +template +void checkPosixError(int err, Args&&... args) { if (UNLIKELY(err != 0)) { - throwSystemError(err, msg); + throwSystemErrorExplicit(err, std::forward(args)...); } } // Check a Linux kernel-style return code (>= 0 on success, negative error // number on error), throw on error. -inline void checkKernelError(ssize_t ret, const char* msg) { +template +void checkKernelError(ssize_t ret, Args&&... args) { if (UNLIKELY(ret < 0)) { - throwSystemError(-ret, msg); + throwSystemErrorExplicit(-ret, std::forward(args)...); } } // Check a traditional Unix return code (-1 and sets errno on error), throw // on error. -inline void checkUnixError(ssize_t ret, const char* msg) { +template +void checkUnixError(ssize_t ret, Args&&... args) { if (UNLIKELY(ret == -1)) { - throwSystemError(msg); + throwSystemError(std::forward(args)...); } } -inline void checkUnixError(ssize_t ret, int savedErrno, const char* msg) { + +template +void checkUnixErrorExplicit(ssize_t ret, int savedErrno, Args&&... args) { if (UNLIKELY(ret == -1)) { - throwSystemError(savedErrno, msg); + throwSystemErrorExplicit(savedErrno, std::forward(args)...); + } +} + +// Check the return code from a fopen-style function (returns a non-nullptr +// FILE* on success, nullptr on error, sets errno). Works with fopen, fdopen, +// freopen, tmpfile, etc. +template +void checkFopenError(FILE* fp, Args&&... args) { + if (UNLIKELY(!fp)) { + throwSystemError(std::forward(args)...); + } +} + +template +void checkFopenErrorExplicit(FILE* fp, int savedErrno, Args&&... args) { + if (UNLIKELY(!fp)) { + throwSystemErrorExplicit(savedErrno, std::forward(args)...); } } diff --git a/folly/File.cpp b/folly/File.cpp index 1262dd1c..aaf109f0 100644 --- a/folly/File.cpp +++ b/folly/File.cpp @@ -15,7 +15,13 @@ */ #include "folly/File.h" + +#include +#include +#include + #include "folly/Format.h" +#include "folly/Exception.h" #include "folly/ScopeGuard.h" #include @@ -37,11 +43,9 @@ File::File(int fd, bool ownsFd) File::File(const char* name, int flags, mode_t mode) : fd_(::open(name, flags, mode)) , ownsFd_(false) { - - if (fd_ < 0) { - throw std::system_error(errno, std::system_category(), - folly::format("open(\"{}\", {:#o}, 0{:#o}) failed", - name, flags, mode).str()); + if (fd_ == -1) { + throwSystemError(folly::format("open(\"{}\", {:#o}, 0{:#o}) failed", + name, flags, mode).fbstr()); } ownsFd_ = true; } @@ -66,15 +70,11 @@ File::~File() { /* static */ File File::temporary() { // make a temp file with tmpfile(), dup the fd, then return it in a File. FILE* tmpFile = tmpfile(); - if (!tmpFile) { - throw std::system_error(errno, std::system_category(), "tmpfile() failed"); - } + checkFopenError(tmpFile, "tmpfile() failed"); SCOPE_EXIT { fclose(tmpFile); }; int fd = dup(fileno(tmpFile)); - if (fd < 0) { - throw std::system_error(errno, std::system_category(), "dup() failed"); - } + checkUnixError(fd, "dup() failed"); return File(fd, true); } @@ -96,7 +96,7 @@ void swap(File& a, File& b) { void File::close() { if (!closeNoThrow()) { - throw std::system_error(errno, std::system_category(), "close() failed"); + throwSystemError("close() failed"); } } @@ -106,4 +106,26 @@ bool File::closeNoThrow() { return r == 0; } +void File::lock() { doLock(LOCK_EX); } +bool File::try_lock() { return doTryLock(LOCK_EX); } +void File::lock_shared() { doLock(LOCK_SH); } +bool File::try_lock_shared() { return doTryLock(LOCK_SH); } + +void File::doLock(int op) { + checkUnixError(flock(fd_, op), "flock() failed (lock)"); +} + +bool File::doTryLock(int op) { + int r = flock(fd_, op | LOCK_NB); + // flock returns EWOULDBLOCK if already locked + if (r == -1 && errno == EWOULDBLOCK) return false; + checkUnixError(r, "flock() failed (try_lock)"); + return true; +} + +void File::unlock() { + checkUnixError(flock(fd_, LOCK_UN), "flock() failed (unlock)"); +} +void File::unlock_shared() { unlock(); } + } // namespace folly diff --git a/folly/File.h b/folly/File.h index e990c91b..71fcc033 100644 --- a/folly/File.h +++ b/folly/File.h @@ -20,6 +20,7 @@ #include #include #include +#include namespace folly { @@ -92,7 +93,26 @@ class File { File(File&&); File& operator=(File&&); + // FLOCK (INTERPROCESS) LOCKS + // + // NOTE THAT THESE LOCKS ARE flock() LOCKS. That is, they may only be used + // for inter-process synchronization -- an attempt to acquire a second lock + // on the same file descriptor from the same process may succeed. Attempting + // to acquire a second lock on a different file descriptor for the same file + // should fail, but some systems might implement flock() using fcntl() locks, + // in which case it will succeed. + void lock(); + bool try_lock(); + void unlock(); + + void lock_shared(); + bool try_lock_shared(); + void unlock_shared(); + private: + void doLock(int op); + bool doTryLock(int op); + // unique File(const File&) = delete; File& operator=(const File&) = delete; @@ -103,6 +123,7 @@ class File { void swap(File& a, File& b); + } // namespace folly #endif /* FOLLY_FILE_H_ */ diff --git a/folly/FileUtil.cpp b/folly/FileUtil.cpp new file mode 100644 index 00000000..8a07bf76 --- /dev/null +++ b/folly/FileUtil.cpp @@ -0,0 +1,165 @@ +/* + * Copyright 2013 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/FileUtil.h" + +#include + +namespace folly { + +int closeNoInt(int fd) { + int r = close(fd); + // Ignore EINTR. On Linux, close() may only return EINTR after the file + // descriptor has been closed, so you must not retry close() on EINTR -- + // in the best case, you'll get EBADF, and in the worst case, you'll end up + // closing a different file (one opened from another thread). + // + // Interestingly enough, the Single Unix Specification says that the state + // of the file descriptor is unspecified if close returns EINTR. In that + // case, the safe thing to do is also not to retry close() -- leaking a file + // descriptor is probably better than closing the wrong file. + if (r == -1 && errno == EINTR) { + r = 0; + } + return r; +} + +namespace { + +// Wrap call to f(args) in loop to retry on EINTR +template +ssize_t wrapNoInt(F f, Args... args) { + ssize_t r; + do { + r = f(args...); + } while (r == -1 && errno == EINTR); + return r; +} + +} // namespace + +ssize_t readNoInt(int fd, void* buf, size_t count) { + return wrapNoInt(read, fd, buf, count); +} + +ssize_t preadNoInt(int fd, void* buf, size_t count, off_t offset) { + return wrapNoInt(pread, fd, buf, count, offset); +} + +ssize_t readvNoInt(int fd, const struct iovec* iov, int count) { + return wrapNoInt(writev, fd, iov, count); +} + +ssize_t writeNoInt(int fd, const void* buf, size_t count) { + return wrapNoInt(write, fd, buf, count); +} + +ssize_t pwriteNoInt(int fd, const void* buf, size_t count, off_t offset) { + return wrapNoInt(pwrite, fd, buf, count, offset); +} + +ssize_t writevNoInt(int fd, const struct iovec* iov, int count) { + return wrapNoInt(writev, fd, iov, count); +} + +ssize_t readFull(int fd, void* buf, size_t count) { + char* b = static_cast(buf); + ssize_t totalBytes = 0; + ssize_t r; + do { + r = read(fd, b, count); + if (r == -1) { + if (errno == EINTR) { + continue; + } + return r; + } + + totalBytes += r; + b += r; + count -= r; + } while (r != 0 && count); // 0 means EOF + + return totalBytes; +} + +ssize_t preadFull(int fd, void* buf, size_t count, off_t offset) { + char* b = static_cast(buf); + ssize_t totalBytes = 0; + ssize_t r; + do { + r = pread(fd, b, count, offset); + if (r == -1) { + if (errno == EINTR) { + continue; + } + return r; + } + + totalBytes += r; + b += r; + offset += r; + count -= r; + } while (r != 0 && count); // 0 means EOF + + return totalBytes; +} + +ssize_t writeFull(int fd, const void* buf, size_t count) { + const char* b = static_cast(buf); + ssize_t totalBytes = 0; + ssize_t r; + do { + r = write(fd, b, count); + if (r == -1) { + if (errno == EINTR) { + continue; + } + return r; + } + + totalBytes += r; + b += r; + count -= r; + } while (r != 0 && count); // 0 means EOF + + return totalBytes; +} + +ssize_t pwriteFull(int fd, const void* buf, size_t count, off_t offset) { + const char* b = static_cast(buf); + ssize_t totalBytes = 0; + ssize_t r; + do { + r = pwrite(fd, b, count, offset); + if (r == -1) { + if (errno == EINTR) { + continue; + } + return r; + } + + totalBytes += r; + b += r; + offset += r; + count -= r; + } while (r != 0 && count); // 0 means EOF + + return totalBytes; +} + +} // namespaces + diff --git a/folly/FileUtil.h b/folly/FileUtil.h new file mode 100644 index 00000000..48e5e712 --- /dev/null +++ b/folly/FileUtil.h @@ -0,0 +1,80 @@ +/* + * Copyright 2013 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. + */ + +#ifndef FOLLY_FILEUTIL_H_ +#define FOLLY_FILEUTIL_H_ + +#include +#include + +namespace folly { + +/** + * Convenience wrappers around some commonly used system calls. The *NoInt + * wrappers retry on EINTR. The *Full wrappers retry on EINTR and also loop + * until all data is written. Note that *Full wrappers weaken the thread + * semantics of underlying system calls. + */ +int closeNoInt(int fd); + +ssize_t readNoInt(int fd, void* buf, size_t n); +ssize_t preadNoInt(int fd, void* buf, size_t n, off_t offset); +ssize_t readvNoInt(int fd, const struct iovec* iov, int count); + +ssize_t writeNoInt(int fd, const void* buf, size_t n); +ssize_t pwriteNoInt(int fd, const void* buf, size_t n, off_t offset); +ssize_t writevNoInt(int fd, const struct iovec* iov, int count); + +/** + * Wrapper around read() (and pread()) that, in addition to retrying on + * EINTR, will loop until all data is read. + * + * This wrapper is only useful for blocking file descriptors (for non-blocking + * file descriptors, you have to be prepared to deal with incomplete reads + * anyway), and only exists because POSIX allows read() to return an incomplete + * read if interrupted by a signal (instead of returning -1 and setting errno + * to EINTR). + * + * Note that this wrapper weakens the thread safety of read(): the file pointer + * is shared between threads, but the system call is atomic. If multiple + * threads are reading from a file at the same time, you don't know where your + * data came from in the file, but you do know that the returned bytes were + * contiguous. You can no longer make this assumption if using readFull(). + * You should probably use pread() when reading from the same file descriptor + * from multiple threads simultaneously, anyway. + */ +ssize_t readFull(int fd, void* buf, size_t n); +ssize_t preadFull(int fd, void* buf, size_t n, off_t offset); +// TODO(tudorb): add readvFull if needed + +/** + * Similar to readFull and preadFull above, wrappers around write() and + * pwrite() that loop until all data is written. + * + * Generally, the write() / pwrite() system call may always write fewer bytes + * than requested, just like read(). In certain cases (such as when writing to + * a pipe), POSIX provides stronger guarantees, but not in the general case. + * For example, Linux (even on a 64-bit platform) won't write more than 2GB in + * one write() system call. + */ +ssize_t writeFull(int fd, const void* buf, size_t n); +ssize_t pwriteFull(int fd, const void* buf, size_t n, off_t offset); +// TODO(tudorb): add writevFull if needed + +} // namespaces + +#endif /* FOLLY_FILEUTIL_H_ */ + diff --git a/folly/test/ExceptionTest.cpp b/folly/test/ExceptionTest.cpp new file mode 100644 index 00000000..e0fe140d --- /dev/null +++ b/folly/test/ExceptionTest.cpp @@ -0,0 +1,97 @@ +/* + * Copyright 2013 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/Exception.h" + +#include +#include + +#include +#include + +namespace folly { namespace test { + +#define EXPECT_SYSTEM_ERROR(statement, err, msg) \ + try { \ + statement; \ + ADD_FAILURE() << "Didn't throw"; \ + } catch (const std::system_error& e) { \ + std::system_error expected(err, std::system_category(), msg); \ + EXPECT_STREQ(expected.what(), e.what()); \ + } catch (...) { \ + ADD_FAILURE() << "Threw a different type"; \ + } + + +TEST(ExceptionTest, Simple) { + // Make sure errno isn't used when we don't want it to, set it to something + // else than what we set when we call Explicit functions + errno = ERANGE; + EXPECT_SYSTEM_ERROR({throwSystemErrorExplicit(EIO, "hello");}, + EIO, "hello"); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({throwSystemErrorExplicit(EIO, "hello", " world");}, + EIO, "hello world"); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({throwSystemError("hello", " world");}, + ERANGE, "hello world"); + + EXPECT_NO_THROW({checkPosixError(0, "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkPosixError(EIO, "hello", " world");}, + EIO, "hello world"); + + EXPECT_NO_THROW({checkKernelError(0, "hello", " world");}); + EXPECT_NO_THROW({checkKernelError(EIO, "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkKernelError(-EIO, "hello", " world");}, + EIO, "hello world"); + + EXPECT_NO_THROW({checkUnixError(0, "hello", " world");}); + EXPECT_NO_THROW({checkUnixError(1, "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkUnixError(-1, "hello", " world");}, + ERANGE, "hello world"); + + EXPECT_NO_THROW({checkUnixErrorExplicit(0, EIO, "hello", " world");}); + EXPECT_NO_THROW({checkUnixErrorExplicit(1, EIO, "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkUnixErrorExplicit(-1, EIO, "hello", " world");}, + EIO, "hello world"); + + std::shared_ptr fp(tmpfile(), fclose); + ASSERT_TRUE(fp != nullptr); + + EXPECT_NO_THROW({checkFopenError(fp.get(), "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkFopenError(nullptr, "hello", " world");}, + ERANGE, "hello world"); + + EXPECT_NO_THROW({checkFopenErrorExplicit(fp.get(), EIO, "hello", " world");}); + errno = ERANGE; + EXPECT_SYSTEM_ERROR({checkFopenErrorExplicit(nullptr, EIO, + "hello", " world");}, + EIO, "hello world"); +} + +}} // namespaces + +int main(int argc, char *argv[]) { + testing::InitGoogleTest(&argc, argv); + google::ParseCommandLineFlags(&argc, &argv, true); + return RUN_ALL_TESTS(); +} + diff --git a/folly/test/FileTest.cpp b/folly/test/FileTest.cpp index 4f47f0fe..3b5acaaa 100644 --- a/folly/test/FileTest.cpp +++ b/folly/test/FileTest.cpp @@ -16,13 +16,20 @@ #include "folly/File.h" +#include + +#include #include #include #include "folly/Benchmark.h" #include "folly/String.h" +#include "folly/Subprocess.h" +#include "folly/experimental/io/FsUtil.h" +#include "folly/experimental/TestUtil.h" using namespace folly; +using namespace folly::test; namespace { void expectWouldBlock(ssize_t r) { @@ -122,3 +129,78 @@ TEST(File, Truthy) { EXPECT_TRUE(false); } } + +TEST(File, Locks) { + typedef std::unique_lock Lock; + typedef boost::shared_lock SharedLock; + + // Find out where we are. + static constexpr size_t pathLength = 2048; + char buf[pathLength + 1]; + int r = readlink("/proc/self/exe", buf, pathLength); + CHECK_ERR(r); + buf[r] = '\0'; + + fs::path helper(buf); + helper.remove_filename(); + helper /= "file_test_lock_helper"; + + TemporaryFile tempFile; + File f(tempFile.fd()); + + enum LockMode { EXCLUSIVE, SHARED }; + auto testLock = [&] (LockMode mode, bool expectedSuccess) { + auto ret = + Subprocess({helper.native(), + mode == SHARED ? "-s" : "-x", + tempFile.path().native()}).wait(); + EXPECT_TRUE(ret.exited()); + if (ret.exited()) { + EXPECT_EQ(expectedSuccess ? 0 : 42, ret.exitStatus()); + } + }; + + // Make sure nothing breaks and things compile. + { + Lock lock(f); + } + + { + SharedLock lock(f); + } + + { + Lock lock(f, std::defer_lock); + EXPECT_TRUE(lock.try_lock()); + } + + { + SharedLock lock(f, boost::defer_lock); + EXPECT_TRUE(lock.try_lock()); + } + + // X blocks X + { + Lock lock(f); + testLock(EXCLUSIVE, false); + } + + // X blocks S + { + Lock lock(f); + testLock(SHARED, false); + } + + // S blocks X + { + SharedLock lock(f); + testLock(EXCLUSIVE, false); + } + + // S does not block S + { + SharedLock lock(f); + testLock(SHARED, true); + } +} + diff --git a/folly/test/FileTestLockHelper.cpp b/folly/test/FileTestLockHelper.cpp new file mode 100644 index 00000000..2ab34390 --- /dev/null +++ b/folly/test/FileTestLockHelper.cpp @@ -0,0 +1,39 @@ +/* + * Copyright 2013 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 +#include + +#include "folly/File.h" + +DEFINE_bool(s, false, "get shared lock"); +DEFINE_bool(x, false, "get exclusive lock"); + +int main(int argc, char *argv[]) { + google::ParseCommandLineFlags(&argc, &argv, true); + google::InitGoogleLogging(argv[0]); + CHECK_EQ(FLAGS_s + FLAGS_x, 1) + << "exactly one of -s and -x must be specified"; + CHECK_EQ(argc, 2); + folly::File f(argv[1], O_RDWR); + bool r; + if (FLAGS_s) { + r = f.try_lock_shared(); + } else { + r = f.try_lock(); + } + return r ? 0 : 42; +} diff --git a/folly/test/SubprocessTest.cpp b/folly/test/SubprocessTest.cpp index bcb163a3..17c145d1 100644 --- a/folly/test/SubprocessTest.cpp +++ b/folly/test/SubprocessTest.cpp @@ -67,7 +67,7 @@ TEST(SimpleSubprocessTest, ShellExitsWithError) { TEST(ParentDeathSubprocessTest, ParentDeathSignal) { // Find out where we are. static constexpr size_t pathLength = 2048; - char buf[pathLength]; + char buf[pathLength + 1]; int r = readlink("/proc/self/exe", buf, pathLength); CHECK_ERR(r); buf[r] = '\0'; -- 2.34.1