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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

I think this patch is mostly ok, but I'm still waiting on responses to two comments I made earlier.



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


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896