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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked an inline comment as done.
sepavloff 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;
+
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896