From 3a4107b072d4e4c7f986b5dcd89282c87eb87592 Mon Sep 17 00:00:00 2001 From: Puyan Lotfi Date: Tue, 6 Aug 2019 05:12:23 +0000 Subject: [PATCH] [clang][DirectoryWatcher] Adding llvm::Expected error handling to create. Prior to this patch Unix style errno error reporting from the inotify layer was used by DirectoryWatcher::create to simply return a nullptr on error. This would generally be ok, except that in LLVM we have much more robust error reporting through the facilities of llvm::Expected. The other critical thing I stumbled across was that the unit tests for DirectoryWatcher were not failing abruptly when inotify_init() was reporting an error, but would continue with the testing and eventually hit a deadlock in a pathological machine state (ie in the unit test, the return nullptr on ::create was ignored). Generally this pathological state never happens on any build bot, so it is totally understandable that it was overlooked, but on a Linux desktop running a dubious desktop environment (which I will not name) there is a chance that said desktop environment could use up enough inotify instances to exceed the user's limit. These are the conditions that led me to hit the deadlock I am addressing in this patch with more robust error handling. With the new llvm::Expected error handling when your system runs out of inotify instances for your user, the unit test will be forced to handle the error or crash and report the issue to the user instead of weirdly deadlocking on a condition variable wait. Differential Revision: https://reviews.llvm.org/D65704 git-svn-id: https://llvm.org/svn/llvm-project/cfe/trunk@367979 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../clang/DirectoryWatcher/DirectoryWatcher.h | 10 ++++++---- .../DirectoryWatcher-not-implemented.cpp | 6 ++++-- .../linux/DirectoryWatcher-linux.cpp | 18 ++++++++++++------ .../mac/DirectoryWatcher-mac.cpp | 18 +++++------------- .../DirectoryWatcher/DirectoryWatcherTest.cpp | 7 +++++++ 5 files changed, 34 insertions(+), 25 deletions(-) diff --git a/include/clang/DirectoryWatcher/DirectoryWatcher.h b/include/clang/DirectoryWatcher/DirectoryWatcher.h index e74443e0bc..d2a1662163 100644 --- a/include/clang/DirectoryWatcher/DirectoryWatcher.h +++ b/include/clang/DirectoryWatcher/DirectoryWatcher.h @@ -11,6 +11,7 @@ #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" #include #include #include @@ -98,10 +99,11 @@ public: : Kind(Kind), Filename(Filename) {} }; - /// Returns nullptr if \param Path doesn't exist or isn't a directory. - /// Returns nullptr if OS kernel API told us we can't start watching. In such - /// case it's unclear whether just retrying has any chance to succeeed. - static std::unique_ptr + /// Asserts if \param Path doesn't exist or isn't a directory. + /// Returns llvm::Expected Error if OS kernel API told us we can't start + /// watching. In such case it's unclear whether just retrying has any chance + /// to succeeed. + static llvm::Expected> create(llvm::StringRef Path, std::function Events, bool IsInitial)> diff --git a/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp b/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp index e330ff06f5..200e540624 100644 --- a/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp +++ b/lib/DirectoryWatcher/default/DirectoryWatcher-not-implemented.cpp @@ -11,9 +11,11 @@ using namespace llvm; using namespace clang; -std::unique_ptr clang::DirectoryWatcher::create( +llvm::Expected> clang::DirectoryWatcher::create( StringRef Path, std::function, bool)> Receiver, bool WaitForInitialSync) { - return nullptr; + return llvm::make_error( + "DirectoryWatcher is not implemented for this platform!", + llvm::inconvertibleErrorCode()); } \ No newline at end of file diff --git a/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp b/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp index 6998efbb5e..8a5e76c521 100644 --- a/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp +++ b/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp @@ -13,6 +13,7 @@ #include "llvm/ADT/ScopeExit.h" #include "llvm/Support/AlignOf.h" #include "llvm/Support/Errno.h" +#include "llvm/Support/Error.h" #include "llvm/Support/Mutex.h" #include "llvm/Support/Path.h" #include @@ -320,16 +321,17 @@ DirectoryWatcherLinux::DirectoryWatcherLinux( } // namespace -std::unique_ptr clang::DirectoryWatcher::create( +llvm::Expected> clang::DirectoryWatcher::create( StringRef Path, std::function, bool)> Receiver, bool WaitForInitialSync) { - if (Path.empty()) - return nullptr; + assert(!Path.empty() && "Path.empty()"); const int InotifyFD = inotify_init1(IN_CLOEXEC); if (InotifyFD == -1) - return nullptr; + return llvm::make_error( + std::string("inotify_init1() error: ") + strerror(errno), + llvm::inconvertibleErrorCode()); const int InotifyWD = inotify_add_watch( InotifyFD, Path.str().c_str(), @@ -340,12 +342,16 @@ std::unique_ptr clang::DirectoryWatcher::create( #endif ); if (InotifyWD == -1) - return nullptr; + return llvm::make_error( + std::string("inotify_add_watch() error: ") + strerror(errno), + llvm::inconvertibleErrorCode()); auto InotifyPollingStopper = SemaphorePipe::create(); if (!InotifyPollingStopper) - return nullptr; + return llvm::make_error( + std::string("SemaphorePipe::create() error: ") + strerror(errno), + llvm::inconvertibleErrorCode()); return llvm::make_unique( Path, Receiver, WaitForInitialSync, InotifyFD, InotifyWD, diff --git a/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp b/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp index ae3cb61416..ade03c2bbc 100644 --- a/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp +++ b/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp @@ -11,16 +11,13 @@ #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/StringRef.h" +#include "llvm/Support/Error.h" #include "llvm/Support/Path.h" #include using namespace llvm; using namespace clang; -static FSEventStreamRef createFSEventStream( - StringRef Path, - std::function, bool)>, - dispatch_queue_t); static void stopFSEventStream(FSEventStreamRef); namespace { @@ -153,8 +150,7 @@ FSEventStreamRef createFSEventStream( StringRef Path, std::function, bool)> Receiver, dispatch_queue_t Queue) { - if (Path.empty()) - return nullptr; + assert(!Path.empty() && "Path.empty()"); CFMutableArrayRef PathsToWatch = [&]() { CFMutableArrayRef PathsToWatch = @@ -205,20 +201,16 @@ void stopFSEventStream(FSEventStreamRef EventStream) { FSEventStreamRelease(EventStream); } -std::unique_ptr clang::DirectoryWatcher::create( +llvm::Expected> clang::DirectoryWatcher::create( StringRef Path, std::function, bool)> Receiver, bool WaitForInitialSync) { dispatch_queue_t Queue = dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL); - if (Path.empty()) - return nullptr; - + assert(!Path.empty() && "Path.empty()"); auto EventStream = createFSEventStream(Path, Receiver, Queue); - if (!EventStream) { - return nullptr; - } + assert(EventStream && "EventStream expected to be non-null.") std::unique_ptr Result = llvm::make_unique(EventStream, Receiver, Path); diff --git a/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp b/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp index 52a6961605..14ef346032 100644 --- a/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp +++ b/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp @@ -284,6 +284,7 @@ TEST(DirectoryWatcherTest, InitialScanSync) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; checkEventualResultWithTimeout(TestConsumer); } @@ -315,6 +316,7 @@ TEST(DirectoryWatcherTest, InitialScanAsync) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/false); + if (!DW) return; checkEventualResultWithTimeout(TestConsumer); } @@ -335,6 +337,7 @@ TEST(DirectoryWatcherTest, AddFiles) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; fixture.addFile("a"); fixture.addFile("b"); @@ -360,6 +363,7 @@ TEST(DirectoryWatcherTest, ModifyFile) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; // modify the file { @@ -390,6 +394,7 @@ TEST(DirectoryWatcherTest, DeleteFile) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; fixture.deleteFile("a"); @@ -411,6 +416,7 @@ TEST(DirectoryWatcherTest, DeleteWatchedDir) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; remove_directories(fixture.TestWatchedDir); @@ -431,6 +437,7 @@ TEST(DirectoryWatcherTest, InvalidatedWatcher) { TestConsumer.consume(Events, IsInitial); }, /*waitForInitialSync=*/true); + if (!DW) return; } // DW is destructed here. checkEventualResultWithTimeout(TestConsumer); -- 2.40.0