[clang][DirectoryWatcher][NFC] Swapping asserts for llvm fatal_error in ::create.

[plotfi]

Summary

Also I went ahead and replaced all the instances of "auto DW = DirectoryWatcher::create" with "llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW = DirectoryWatcher::create(" to make it more clear that DirectoryWatcher::create is returning an llvm::Expected (along wit a comment to make it extra clear).

[gribozavr]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

Call llvm::cantFail and there will be no need to explain anything.

[plotfi]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

I explained this in the other patch:

cantFail is arguably worse. I want this test to tell me the cause of the crash (in this case, exceeding the inotify limit). What I want to happen is:

Note: Google Test filter = DirectoryWatcherTest.AddFiles
[==========] Running 1 test from 1 test case.
[----------] Global test environment set-up.
[----------] 1 test from DirectoryWatcherTest
[ RUN      ] DirectoryWatcherTest.AddFiles
Expected<T> must be checked before access or destruction.
Unchecked Expected<T> contained error:
inotify_init1() error: out of inotify entries #0 0x0000000000246b8f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x00000000002450d2 llvm::sys::RunSignalHandlers() /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x0000000000247268 SignalHandler(int) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
 #3 0x00007f17924eb890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x00007f1790f94e97 gsignal /build/glibc-OTsEL5/glibc-
...

Wrapping the llvm::Expected in a cantFail hides this message, and instead you get the much less useful:

Note: Google Test filter = DirectoryWatcherTest.AddFiles
[==========] Running 1 test from 1 test case.                                                                                                                                                                                                
[----------] Global test environment set-up.
[----------] 1 test from DirectoryWatcherTest
[ RUN      ] DirectoryWatcherTest.AddFiles
Failure value returned from cantFail wrapped call
UNREACHABLE executed at /mnt/nvme0/llvm-project/llvm/include/llvm/Support/Error.h:707!
 #0 0x0000000000246e0f llvm::sys::PrintStackTrace(llvm::raw_ostream&) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:533:13
 #1 0x0000000000245352 llvm::sys::RunSignalHandlers() /mnt/nvme0/llvm-project/llvm/lib/Support/Signals.cpp:69:18
 #2 0x00000000002474e8 SignalHandler(int) /mnt/nvme0/llvm-project/llvm/lib/Support/Unix/Signals.inc:385:1
 #3 0x00007f20c64a8890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890)
 #4 0x00007f20c4f51e97 gsignal /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0
 #5 0x00007f20c4f53801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0
 #6 0x0000000000232b31 (build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x232b31)

I specifically _want_ llvm::Expected to propagate the error up.
I can drop the comments, because it should be fairly obvious here that DW is an llvm::Expected since I got rid of the autos.

To be clear, wrapping llvm::Expected in cantFail goes against the entire point of why I added this more robust error handling.
If someone's machine is failing on this test because they've exceeded their inotify limit, I would really like the message produced from strerror(errno) to bubble up to the top so that it is obvious to them that they need to either kill some tasks on their machine or up their inotify limit.

[gribozavr]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

The automated checking in llvm::Expected is not guaranteed to happen (it is behind LLVM_ENABLE_ABI_BREAKING_CHECKS). If you are not happy with the error from cantFail I think everyone would appreciate an improvement there.

I specifically _want_ llvm::Expected to propagate the error up.

I don't understand. It is not propagating anything up, it is just crashing in the destructor.

[plotfi]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

Also, I have to point out here: cantFail is exactly the wrong tool here because this code can in fact fail if your system has exceeded its inotify limits.

[gribozavr]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

For the purposes of the test, the call can't fail. If the call fails, the test fails.

How about ASSERT_TRUE(DW)?

[plotfi]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

I meant, it forces the code to either have to takeError and propagate the error up or it will crash in the destructor. As far as I can tell that's how it's supposed to work. I want the error produced inside DirectoryWatcher::create to show up in the user's console.

[compnerd]

@gribozavr I think that this usage here is actually useful because it
a) tests the actual behaviour
b) provides example code for other users

The check here ensures that the rest of the code is properly executed, *but* because the error is not actually consumed (you need to explicitly invoke .takeError()), llvm::Expected will actually tell you that a fatal error has occurred at this point and what the error was. It provides a much clearer error message than the explicit llvm_unreachable IMO.

[gribozavr]

clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

323 ↗

(On Diff #213716)

I want the error produced inside DirectoryWatcher::create to show up in the user's console.

But the destructor of llvm::Expected is not guaranteed to do that. It will only detect (and print) the error under LLVM_ENABLE_ABI_BREAKING_CHECKS. This is exactly why I'm suggesting various alternative solutions.

[lhames]

I think the right line is:

logAllUnhandledErrors(DW.takeError(), errs(), "");

It's a bit wordy, but will log a sensible error and works with and without ENABLE_ABI_BREAKING_CHECKS enabled.

I'm not against improving the output for cantFail, nor totally against abusing cantFail in unit tests, but Puyan's right that it would be abuse: in production code cantFail should only be used at call sites where you know from context that the call definitely can't fail (e.g. subsequent calls to an API that can only fail on the first call).

[plotfi]

In D65829#1617850, @lhames wrote:

I think the right line is:

logAllUnhandledErrors(DW.takeError(), errs(), "");

It's a bit wordy, but will log a sensible error and works with and without ENABLE_ABI_BREAKING_CHECKS enabled.

I'm not against improving the output for cantFail, nor totally against abusing cantFail in unit tests, but Puyan's right that it would be abuse: in production code cantFail should only be used at call sites where you know from context that the call definitely can't fail (e.g. subsequent calls to an API that can only fail on the first call).

I am happy with this. Thanks @lhames !

[compnerd]

@lhames - I like the logAllUnhandledErrors!

[jkorous]

I was about to suggest something similar. SGTM!

[plotfi]

Updated to use logAllUnhandledErrors

[plotfi]

In D65829#1617864, @compnerd wrote:

@lhames - I like the logAllUnhandledErrors!

@lhames @compnerd @jkorous

Only downside to using logAllUnhandledErrors over just letting the llvm::Expected's destructor handle the error print out is that logAllUnhandledErrors only logs the errors. It doesn't crash the rest of the deadlocking DirectoryWatcher test. So I had to include logAllUnhandledErrors along with exit(EXIT_FAILURE).

Anyhow, it seems we have consensus so I will land this.

[jkorous]

Thanks Puyan!

cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h

struct Event {
     /// Filename that this event is related to or an empty string in
     /// case this event is related to the watched directory itself.
     std::string Filename;
 
     Event(EventKind Kind, llvm::StringRef Filename)
         : Kind(Kind), Filename(Filename) {}
   };
 
-  /// Asserts if \param Path doesn't exist or isn't a directory.
+  /// llvm fatal_error 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.
+  /// to succeed.
   static llvm::Expected<std::unique_ptr<DirectoryWatcher>>
   create(llvm::StringRef Path,
          std::function<void(llvm::ArrayRef<DirectoryWatcher::Event> Events,
                             bool IsInitial)>
              Receiver,
          bool WaitForInitialSync);
 
   virtual ~DirectoryWatcher() = default;

cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp

 }
 
 } // namespace
 
 llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::create(
     StringRef Path,
     std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
     bool WaitForInitialSync) {
-  assert(!Path.empty() && "Path.empty()");
+  if (Path.empty())
+    llvm::report_fatal_error(
+        "DirectoryWatcher::create can not accept an empty Path.");
 
   const int InotifyFD = inotify_init1(IN_CLOEXEC);
   if (InotifyFD == -1)
     return llvm::make_error<llvm::StringError>(
         std::string("inotify_init1() error: ") + strerror(errno),
         llvm::inconvertibleErrorCode());
 
   const int InotifyWD = inotify_add_watch(

cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp

if (!Events.empty()) {
     ctx->Receiver(Events, /*IsInitial=*/false);
   }
 }
 
 FSEventStreamRef createFSEventStream(
     StringRef Path,
     std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
     dispatch_queue_t Queue) {
-  assert(!Path.empty() && "Path.empty()");
+  if (Path.empty())
+    return nullptr;
 
   CFMutableArrayRef PathsToWatch = [&]() {
     CFMutableArrayRef PathsToWatch =
         CFArrayCreateMutable(nullptr, 0, &kCFTypeArrayCallBacks);
     CFStringRef CfPathStr =
         CFStringCreateWithBytes(nullptr, (const UInt8 *)Path.data(),
                                 Path.size(), kCFStringEncodingUTF8, false);
     CFArrayAppendValue(PathsToWatch, CfPathStr);

 
 llvm::Expected<std::unique_ptr<DirectoryWatcher>> clang::DirectoryWatcher::create(
     StringRef Path,
     std::function<void(llvm::ArrayRef<DirectoryWatcher::Event>, bool)> Receiver,
     bool WaitForInitialSync) {
   dispatch_queue_t Queue =
       dispatch_queue_create("DirectoryWatcher", DISPATCH_QUEUE_SERIAL);
 
-  assert(!Path.empty() && "Path.empty()");
+  if (Path.empty())
+    llvm::report_fatal_error(
+        "DirectoryWatcher::create can not accept an empty Path.");
+
   auto EventStream = createFSEventStream(Path, Receiver, Queue);
   assert(EventStream && "EventStream expected to be non-null");
 
   std::unique_ptr<DirectoryWatcher> Result =
       llvm::make_unique<DirectoryWatcherMac>(EventStream, Receiver, Path);
 
   // We need to copy the data so the lifetime is ok after a const copy is made
   // for the block.

cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp

VerifyingConsumer TestConsumer{
       // We have to ignore these as it's a race between the test process
       // which is scanning the directory and kernel which is sending
       // notification.
       {{EventKind::Modified, "a"},
        {EventKind::Modified, "b"},
        {EventKind::Modified, "c"}}
       };
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/true);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, InitialScanAsync) {
   DirectoryWatcherTestFixture fixture;
 
   fixture.addFile("a");
   fixture.addFile("b");
   fixture.addFile("c");
 
   VerifyingConsumer TestConsumer{
       {{EventKind::Modified, "a"},
        {EventKind::Modified, "b"},
        {EventKind::Modified, "c"}},
       {},
       // We have to ignore these as it's a race between the test process
       // which is scanning the directory and kernel which is sending
       // notification.
       {{EventKind::Modified, "a"},
        {EventKind::Modified, "b"},
        {EventKind::Modified, "c"}}
        };
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/false);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, AddFiles) {
   DirectoryWatcherTestFixture fixture;
 
   VerifyingConsumer TestConsumer{
       {},
       {{EventKind::Modified, "a"},
        {EventKind::Modified, "b"},
        {EventKind::Modified, "c"}}};
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/true);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   fixture.addFile("a");
   fixture.addFile("b");
   fixture.addFile("c");
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, ModifyFile) {
   DirectoryWatcherTestFixture fixture;
 
   fixture.addFile("a");
 
   VerifyingConsumer TestConsumer{
       {{EventKind::Modified, "a"}},
       {{EventKind::Modified, "a"}},
       {{EventKind::Modified, "a"}}};
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/true);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   // modify the file
   {
     std::error_code error;
     llvm::raw_fd_ostream bStream(fixture.getPathInWatched("a"), error,
                                  CD_OpenExisting);
     assert(!error);
     bStream << "foo";
   }
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, DeleteFile) {
   DirectoryWatcherTestFixture fixture;
 
   fixture.addFile("a");
 
   VerifyingConsumer TestConsumer{
       {{EventKind::Modified, "a"}},
       {{EventKind::Removed, "a"}},
       {{EventKind::Modified, "a"}, {EventKind::Removed, "a"}}};
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/true);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   fixture.deleteFile("a");
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, DeleteWatchedDir) {
   DirectoryWatcherTestFixture fixture;
 
   VerifyingConsumer TestConsumer{
       {},
       {{EventKind::WatchedDirRemoved, ""},
        {EventKind::WatcherGotInvalidated, ""}}};
 
-  auto DW = DirectoryWatcher::create(
+  llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+      DirectoryWatcher::create(
           fixture.TestWatchedDir,
           [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                           bool IsInitial) {
             TestConsumer.consume(Events, IsInitial);
           },
           /*waitForInitialSync=*/true);
-  if (!DW) return;
+  if (!DW) {
+    logAllUnhandledErrors(
+        DW.takeError(), llvm::errs(),
+        "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+    exit(EXIT_FAILURE);
+  }
 
   remove_directories(fixture.TestWatchedDir);
 
   checkEventualResultWithTimeout(TestConsumer);
 }
 
 TEST(DirectoryWatcherTest, InvalidatedWatcher) {
   DirectoryWatcherTestFixture fixture;
 
   VerifyingConsumer TestConsumer{
       {}, {{EventKind::WatcherGotInvalidated, ""}}};
 
   {
-    auto DW = DirectoryWatcher::create(
+    llvm::Expected<std::unique_ptr<DirectoryWatcher>> DW =
+        DirectoryWatcher::create(
             fixture.TestWatchedDir,
             [&TestConsumer](llvm::ArrayRef<DirectoryWatcher::Event> Events,
                             bool IsInitial) {
               TestConsumer.consume(Events, IsInitial);
             },
             /*waitForInitialSync=*/true);
-    if (!DW) return;
+    if (!DW) {
+      logAllUnhandledErrors(
+          DW.takeError(), llvm::errs(),
+          "DirectoryWatcherTest Failure on DirectoryWatcher::create(): ");
+      exit(EXIT_FAILURE);
+    }
   } // DW is destructed here.
 
   checkEventualResultWithTimeout(TestConsumer);
-}
+}