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

[Serge Pavlov via Phabricator via llvm-commits]

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


================
Comment at: llvm/include/llvm/Support/FileSystem.h:1138
+/// <em>errc::success</em>, the process may read or write the file assuming no
+/// other processes may change it. Any other process that calls this function
+/// would get <em>errc::no_lock_available</em> until the process owning the lock
----------------
labath wrote:
> labath wrote:
> > I find this "may change it" part very ambiguous. Did you mean that the process may assume that no other process changes that file (because the operating system guarantees that) or that the process must assume that no other process will change it (because there is no mechanism in the OS to prevent it).
> > 
> > Note that if my thoughts on WriteFile above are true, then neither of the two interpretations are correct, and all we can say is something like. "Attempts to lock the file by other processes will fail/block, but the caller should not assume that the file cannot be modified by uncooperating processes who access it without locking."
> It would be nice to get this clarified as well..
I changed the wording in this point.


================
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:
> 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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896