[PATCH] D78896: [Support] Add file lock/unlock functions

[Pavel Labath via Phabricator via llvm-commits]

labath added inline comments.


================
Comment at: llvm/unittests/Support/Path.cpp:2054-2109
+  std::mutex Start2M;
+  std::condition_variable Start2CV;
+  bool Start2 = false;
+  std::mutex LockM;
+  std::condition_variable LockCV;
+  bool Locked = false;
+
----------------
sepavloff wrote:
> labath wrote:
> > sepavloff wrote:
> > > labath wrote:
> > > > labath wrote:
> > > > > I get the impression this code is much more complicated then needed. There's a lot of synchronization going on but it still does not guarantee the that the file is unlocked while the other thread is inside the `tryLock` call (my goal was to get coverage for the while loop). How about something like:
> > > > > ```
> > > > > EC = fs::tryLockFile(FD1);
> > > > > ASSERT_NO_ERROR(EC);
> > > > > EC = fs::tryLockFile(FD2);
> > > > > ASSERT_ERROR(EC);
> > > > > std::thread LockThread([&] { EC2 = fs::tryLockFile(FD2, std::chrono::minutes(1)); });
> > > > > std::this_thread::sleep_for(std::chrono::seconds(1));
> > > > > EC = fs::unlockFile(FD1);
> > > > > ASSERT_NO_ERROR(EC);
> > > > > LockThread.join();
> > > > > ASSERT_NO_ERROR(EC2);
> > > > > EC = fs::unlockFile(FD2);
> > > > > ASSERT_NO_ERROR(EC);
> > > > > ```
> > > > > It still does not guarantee that the other thread is inside the `tryLockFile` call, but it comes as close as we can get, and it avoids all the condition_variable overhead.
> > > > Waiting on a response to this. I still feel that this can be organized in a much simpler way without using so many explicit synchronization primitives.
> > > I put a diagram explaining what the test does. Actually there are two events, which ensures that the first attempt to lock file occurs in Thread2 after Thread1 locks the file but before it releases it. So the two calls to `tryLockFile` checks both cases, successful and unsuccessful. Each event requires a mutex and a condvar, so we have 4 synchronization objects. Simpler variants do not guarantee checking the both cases. 
> > I believe that each of these events requires *synchronization*, but a condition variable is not the only way to achieve that. Starting and joining a thread is also a form of synchronization, and it is much simpler (and something you have to do anyway.
> > 
> > So, instead of starting a thread which, as a first order of business blocks on a condition variable, you could just delay starting the thread until such a time that the condition would be satisfied. Basically -- remove `cv.wait_for` and replace `cv.notify` with the creation of the thread object. Then, instead of waiting for the other thread to unblock you, you can just `join` it.
> > 
> > For the second condition variable, you just create a fresh thread again. Having the thread bodies be smaller would make the code much easier to follow, and it's not like we have to worry about the performance overhead of creating a bunch of small threads here...
> Starting the second thread inside the first instead of waiting event indeed makes the test more compact, I rewrote the test accordingly.
> 
> As for the second lock try, which happens after the file is unlocked, using new thread makes the logic more obscure. I moved synchronization stuff into the new class `Event`, it must make the test shorter and clearer.
Creating the Event class does make it a bit better, but I still maintain that this test is too complicated for what it really tests. Take a look at the following test:
```
TEST_F(FileSystemTest, lockFileThread) {
#if LLVM_ENABLE_THREADS
  int FD1, FD2;
  SmallString<64> TempPath;
  ASSERT_NO_ERROR(fs::createTemporaryFile("test", "temp", FD1, TempPath));
  FileRemover Cleanup(TempPath);
  ASSERT_NO_ERROR(fs::openFileForReadWrite(TempPath, FD2, fs::CD_OpenExisting,
                                           fs::OF_Append));


  ASSERT_NO_ERROR(fs::tryLockFile(FD1));
  ASSERT_ERROR(fs::tryLockFile(FD2));
  std::future<std::error_code> Future = std::async(std::launch::async, [&] {
    return fs::tryLockFile(FD2, std::chrono::seconds(5));
  });
  ASSERT_NO_ERROR(fs::unlockFile(FD1));
  ASSERT_NO_ERROR(Future.get());
  fs::unlockFile(FD2);

  ASSERT_NO_ERROR(fs::tryLockFile(FD1));
  ASSERT_ERROR(fs::tryLockFile(FD2));
  Future = std::async(std::launch::async, [&] { return fs::lockFile(FD2); });
  ASSERT_NO_ERROR(fs::unlockFile(FD1));
  ASSERT_NO_ERROR(Future.get());
  fs::unlockFile(FD2);

#endif
}
```
It tests the same thing as the test you wrote -- I obtained by applying series of semantics-preserving simplifications to it. This included fairly simple things like:
- inlining
- replacing patterns like std::thread(foo).join() with direct calls to `foo`
- moving code which does not block outside of a thread -- e.g. asserting that a lock attempt fails does not need to be done in a separate thread because it does not block. Only the blocking calls do.
- replacing a thread consisting of a single expression with a call to `std::async`
- removing unused variables produced by all of this

However, the end result is a test which is about 3 times shorter than the original (28 lines vs 88), and it's almost linear -- each parallel section is only three lines long. I think it'd be pretty hard to argue that this is not more readable than the original.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896