There are cases where the DirectoryWatcherTests just hang in a deadlock when there are inotify limits hit, with no error reporting. This patch is a first attempt to resolve these issues by bubbling up errors to the user.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp | ||
---|---|---|
31 | This include should be removed. |
Rather than silently ignoring tests when the DirectoryWatcher isn't created, can you please print an error message and exit with an error code to indicate the test failed?
clang/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp | ||
---|---|---|
329 | I think that this is pointless. Just make it a constant string: "no path specified". Path is a user specified parameter, errno doesn't tell us anything. | |
clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
213 | Similar. |
BTW, I think that we should add a test case to ensure that we see the error in the case that the inotify fds are exhausted. We should be able to create a process and set the limit for that process to 0/1 and use that to trigger the failure.
Please just update the comment, otherwise LGTM.
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
283 | Ah, my bad - I just took a better look at Expected<> and you're right. |
clang/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
101–102 | Ahh, Yes. | |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
283 | Nah, the way llvm::Expected works is that if the error isn't consumed then it will blow up in the destructor. So if it is an error, returning will cause the destructor to crash the program and print the error implicitly. Very nice error handling mechanism you ask me :-) |
clang/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
---|---|---|
213 | Should this just assert as well? |
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
102 ↗ | (On Diff #213521) | I don't see where this logic is implemented. Also, LLVM is often built with assertions disabled, so "asserts" can't be a part of the contract. |
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
153 ↗ | (On Diff #213521) | No need to repeat the condition in the && "..." part. assert does that by default. Use an extra string to explain the assertion to the reader. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
287 ↗ | (On Diff #213521) | Why? This is a test -- ignoring errors would only hide them from developers. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
283 | And crashing would be much better in a test. The test should test the DirectoryWatcher, not just be graceful about error handling. |
clang/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
283 | Test was in a deadlock, see commit message. This fixes that. |
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
102 ↗ | (On Diff #213521) | Please be more specific. What logic are you talking about? |
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
153 ↗ | (On Diff #213521) | This is std assert. Are you referring to a different assert? If @compnerd is ok with changing the assert into an llvm::Expected I can do that. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
287 ↗ | (On Diff #213521) | Please see how llvm::Expected works. This does not ignore or hide anything. |
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
102 ↗ | (On Diff #213521) | We shouldn't assert. Just return an error and let client code handle it in any way it wants. |
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
153 ↗ | (On Diff #213521) | I think the suggestion is just assert(!Path.empty()); |
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
102 ↗ | (On Diff #213521) | I'm saying that it is not appropriate to document the API as assert()ing about something, because it is not a part of the contract. You can say that the API *requires* the Path to be non-empty. Or you can call llvm_fatal_error explicitly if it is empty, and then you can document it. Or you can return an error like jkorous said. However, assertions are not a part of the API contract. |
cfe/trunk/lib/DirectoryWatcher/mac/DirectoryWatcher-mac.cpp | ||
153 ↗ | (On Diff #213521) | Right. The standard assert already prints the expression -- in fact, this is how the && "xyz" part gets printed when assertion fails -- assert prints the expression that failed. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
287 ↗ | (On Diff #213521) | I think I know how llvm::Expected works. The problem that this check and return hides is that if DirectoryWatcher::create returns an error, the rest of the test is silently slipped. The test should be using something like cantFail instead. |
cfe/trunk/include/clang/DirectoryWatcher/DirectoryWatcher.h | ||
---|---|---|
102 ↗ | (On Diff #213521) | That makes sense. Lemme think about which is best, and I'll amend this diff. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
287 ↗ | (On Diff #213521) | If DirectoryWatcher returns an error and you hit the early return, without taking the error the Expected destructor will print a callstack and halt the program. I can check this again, but I am quite sure that without invoking something like takeError you will hit the crash in the case of an early return. |
cfe/trunk/lib/DirectoryWatcher/linux/DirectoryWatcher-linux.cpp | ||
---|---|---|
329 ↗ | (On Diff #213521) | @gribozavr As an example, let say we stuck the following right here: return llvm::make_error<llvm::StringError>("llvm::Expected will blow up", llvm::inconvertibleErrorCode()); When running: DirectoryWatcherTests --gtest_filter=DirectoryWatcherTest.AddFiles You will get: 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: llvm::Expected will blow up #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 0x00007fa2a284d890 __restore_rt (/lib/x86_64-linux-gnu/libpthread.so.0+0x12890) #4 0x00007fa2a12f6e97 gsignal /build/glibc-OTsEL5/glibc-2.27/signal/../sysdeps/unix/sysv/linux/raise.c:51:0 #5 0x00007fa2a12f8801 abort /build/glibc-OTsEL5/glibc-2.27/stdlib/abort.c:81:0 #6 0x00000000002229db llvm::raw_ostream::operator<<(llvm::StringRef) /mnt/nvme0/llvm-project/llvm/include/llvm/Support/raw_ostream.h:176:7 #7 0x00000000002229db llvm::raw_ostream::operator<<(char const*) /mnt/nvme0/llvm-project/llvm/include/llvm/Support/raw_ostream.h:186:0 #8 0x00000000002229db llvm::Expected<std::unique_ptr<clang::DirectoryWatcher, std::default_delete<clang::DirectoryWatcher> > >::fatalUncheckedExpected() const /mnt/nvme0/llvm-project/llvm/include/llvm/Support/Error.h:661:0 #9 0x000000000021e148 (build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x21e148) #10 0x000000000024cbaa testing::internal::UnitTestImpl::os_stack_trace_getter() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4919:7 #11 0x000000000024cbaa testing::Test::Run() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2481:0 #12 0x000000000024d840 testing::internal::UnitTestImpl::os_stack_trace_getter() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4919:7 #13 0x000000000024d840 testing::TestInfo::Run() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2660:0 #14 0x000000000024dee7 testing::TestCase::Run() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:2773:44 #15 0x00000000002557b7 testing::internal::UnitTestImpl::RunAllTests() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4648:24 #16 0x0000000000255411 testing::UnitTest::Run() /mnt/nvme0/llvm-project/llvm/utils/unittest/googletest/src/gtest.cc:4257:10 #17 0x000000000024859b main /mnt/nvme0/llvm-project/llvm/utils/unittest/UnitTestMain/TestMain.cpp:50:3 #18 0x00007fa2a12d9b97 __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:344:0 #19 0x000000000021b02a _start (build/tools/clang/unittests/DirectoryWatcher/./DirectoryWatcherTests+0x21b02a) Aborted (core dumped) This is preferable to deadlocking. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
287 ↗ | (On Diff #213521) | That's good -- but I think a cantFail call would be more readable. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
287 ↗ | (On Diff #213521) | Ah, you should have said so then. Except this is arguably worse. I want this test to tell me that 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 can add comments assuring that llvm::Expected will dump an error prior to halting, but I specifically _want_ llvm::Expected to propagate up what perror would print to the console otherwise. |
cfe/trunk/unittests/DirectoryWatcher/DirectoryWatcherTest.cpp | ||
---|---|---|
287 ↗ | (On Diff #213521) | To be clear, I want the error to tell the programmer what is going on so that they can either figure out what is eating up their machine resources and either modify a sysctl entry or kill some process. |
@jkorous Can you lemme know if you're happy with https://reviews.llvm.org/D65829.
@gribozavr lemme also know any other feedback in D65829, as this diff here is already closed.
Could you please update the comments?