[PATCH] D79066: [Support] Class to facilitate file locking

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked 6 inline comments as done.
sepavloff added inline comments.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1168
+/// RAII class that facilitates file locking.
+class FileLocker {
+  int FD;             ///< Locked file handle.
----------------
labath wrote:
> sepavloff wrote:
> > dblaikie wrote:
> > > sepavloff wrote:
> > > > dblaikie wrote:
> > > > > Perhaps this type should be boolean testable (for the error), or using a factory function to create one that returns llvm::Error (wrapped around the error_code) on failure:
> > > > > 
> > > > > ```
> > > > > {
> > > > > Expected<FileLocker> F = TryLock(FD, Timeout);
> > > > > if (!F) {
> > > > > }
> > > > > ...
> > > > > }
> > > > > ```
> > > > > 
> > > > > Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> > > > > Perhaps this type should be boolean testable (for the error)
> > > > 
> > > > The class has method `locked`, which can be used to check if lock was successfully acquired, and method `error` for those applications that need more information than just the fact of failure.
> > > > 
> > > > > Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> > > > 
> > > > If the failure to unlock needs to be handled, the user would use method `unlock` provided by this class. In most expected use cases however this failure need not be handled. User defines RAII object and do things that require exclusive access. Upon destruction lock is released and if something goes wrong at that moment, this fact is just ignored.
> > > >> Perhaps this type should be boolean testable (for the error)
> > > > The class has method locked, which can be used to check if lock was successfully acquired, and method error for those applications that need more information than just the fact of failure.
> > > 
> > > Yeah, I think it'd be better if this was only accessible via a free function that returns Expected<FileLocker> so that the Error must be checked for/can't be accidentally ignored.
> > > 
> > > >> Hmm - if unlocking can fail, and that failure needs to be handled, this doesn't seem suitable for an RAII type device...
> > > > If the failure to unlock needs to be handled, the user would use method unlock provided by this class. In most expected use cases however this failure need not be handled. User defines RAII object and do things that require exclusive access. Upon destruction lock is released and if something goes wrong at that moment, this fact is just ignored.
> > > 
> > > Fair enough/that sounds OK then. But it should return llvm::Error to make sure errors are handled/not ignored
> > Added function `tryLock` for using `Expected<FileLocker>`
> Looking at the unit test I realized that `fs::tryLockFile` and `fs::tryLock` do quite different things, but that this difference is not obvious from the function names. The way I would deal with that is by making `tryLock` a static member function `fs::FileLocker::tryLock` (and then make the FileLocker constructor private), but I see that David has requested a free function. David, what are your thoughts on that?
I renamed the function into `getLock`. It looks not so categorical as `lockFile` and is unlikely to be confused with `tryLockFile`. Using static member functions leads to long names, it decreases readability.


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1218
+inline Expected<FileLocker>
+tryLock(int FD, std::chrono::milliseconds T = std::chrono::milliseconds(1000)) {
+  FileLocker Lock(FD, T);
----------------
labath wrote:
> sepavloff wrote:
> > labath wrote:
> > > I don't think there should be a default value for the timeout for two reasons:
> > > - it makes it very unobvious that there is any timing out happening (`tryLock(FD)` looks a lot like `mutex.try_lock()` which only does an instantaneous lock check with no retries or time outs)
> > > - the appropriate value of the timeout sounds like something that is going to be very dependent on the exact use case, and the person using it should make a deliberate choice about the value (and explain it). (Speaking of that, I am still not sure your use case really needs timeouts, though that's more of a discussion for the other patch.)
> > You are right, removed default argument.
> > 
> > Timeout is a form of protection against deadlocks. For the particular task of writing to log file infinite wait time is also acceptable, but in other use cases (such as unit tests) timeouts may be convenient.
> I don't think unit tests are a good motivating example. There are plenty of ways a test can not terminate without locking files (or anything else). The solution to that is to have a (fairly high) timeout at the level of the test harness. That we already have.
> 
> So if the unit tests are the main motivation for introducing these timeouts, I think we should remove them, as they add a fair amount of complexity (and flakyness) to the equation.
> 
> Even for the clang use case, whether one prefers a deadlock over the program silently doing the wrong thing (perhaps when the system is under heavy load, like it is when running the test suite), depends on what exactly is he trying to achieve. But that's something I don't feel qualified to judge...
Unit tests are not a motivation for using timeouts, but they demonstrates the benefits. If users can specify timeouts, they have possibility to handle deadlocks. It is up to user to handle such case or not, but the possibility exists. Without timeouts user must behave as if deadlocks never happen. It limits usability of the solution and it is the motivation.


================
Comment at: llvm/unittests/Support/Path.cpp:42-44
+#include <condition_variable>
 #include <mutex>
+#include <thread>
----------------
labath wrote:
> I guess these leaked in from the dependant patch somehow...
Yes, rebased the patch properly.


================
Comment at: llvm/unittests/Support/Path.cpp:2127
+
+  if (Expected<fs::FileLocker> L = fs::tryLock(FD, std::chrono::seconds(1))) {
+    EC = fs::tryLockFile(Stream.getFD());
----------------
labath wrote:
> A cleaner way to write this would be:
> ```
> Expected<fs::FileLocker> L = fs::tryLock(...);
> ASSERT_THAT_EXPECTED(L, Succeeded());
> // do the other checks..
> ```
Thank you for the idea!
However the variant with lock object local to `if` is recommended way to make locks, so at least one example should be present. But other I reworked in the proposed way.


================
Comment at: llvm/unittests/Support/Path.cpp:2147-2164
+  if (Expected<fs::FileLocker> L1 = fs::tryLock(FD, std::chrono::seconds(1))) {
+    if (Expected<fs::FileLocker> L2 = fs::tryLock(Stream.getFD(),
+                                                  std::chrono::seconds(1))) {
+      ADD_FAILURE();
+    } else {
+      handleAllErrors(std::move(L2.takeError()), [&](ErrorInfoBase &EIB) {});
+    }
----------------
labath wrote:
> ```
> L = fs::tryLock(FD);
> ASSERT_THAT_EXPECTED(L, Succeeded());
> EXPECT_THAT_EXPECTED(fs::tryLock(Stream.getFD()), Failed());
> ASSERT_NO_ERROR(L->unlock());
> EXPECT_THAT_EXPECTED(fs::tryLock(Stream.getFD()), Succeeded());
> ```
Reworked this place but with small change: temporary values are left in variables. It is convenient in tests, as if something goes wrong, it is easier to find reason of the problem in debugger.


================
Comment at: llvm/unittests/Support/Path.cpp:2171-2182
+  if (Expected<fs::FileLocker> L = fs::tryLock(FD, std::chrono::seconds(1))) {
+    EC = L->unlock();
+    ASSERT_NO_ERROR(EC);
+  } else {
+    handleAllErrors(std::move(L.takeError()), [&](ErrorInfoBase &EIB) {});
+    ADD_FAILURE();
+  }
----------------
labath wrote:
> This could be rewritten in a similar manner, though it's not clear to me what is it testing that wasn't already covered by the tests above.
Removed this test. Previously it checked double unlock, but in the new implementation of `Locker` it is not possible to diagnose such case.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D79066/new/

https://reviews.llvm.org/D79066