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

[Pavel Labath via Phabricator via llvm-commits]

labath added a comment.

This looks good to me too. Just some stylistic comments inline.



================
Comment at: llvm/include/llvm/Support/FileSystem.h:1136-1137
+///
+/// The function locks the entire file for exclusive access by the calling
+/// process.
+///
----------------
Given the very different locking semantics on different OSs, it would be good to explicitly mention what this functions does (or does not) promise. Something along the lines of that this is guaranteed to work only if other processes also try to lock the file the same way, and that it is unspecified whether holding a lock prevents another process from modifying the said file.


================
Comment at: llvm/lib/Support/Unix/Path.inc:34
 
+#include<sys/file.h>
 #include <dirent.h>
----------------
add space before `<`


================
Comment at: llvm/lib/Support/Unix/Path.inc:1060-1068
+    if (::flock(FD, LOCK_EX | LOCK_NB) == -1) {
+      int Error = errno;
+      if (Error == EWOULDBLOCK) {
+        usleep(1000);
+        continue;
+      }
+      return std::error_code(Error, std::generic_category());
----------------
You could reduce nesting here by flipping the condition: `if (flock(...) == 0) return std::error_code();`


================
Comment at: llvm/lib/Support/Windows/Path.inc:1272
+      return std::error_code();
+    } else {
+      DWORD Error = ::GetLastError();
----------------
No `else` after `return`.


================
Comment at: llvm/lib/Support/Windows/Path.inc:1290
+    return std::error_code();
+  else
+    return mapWindowsError(::GetLastError());
----------------
same here.


================
Comment at: llvm/unittests/Support/Path.cpp:1938-1939
+#ifdef _WIN32
+// Windows refuses lock request if file region is already locked by the same
+// process. POSIX system in this case updates the existing lock.
+TEST_F(FileSystemTest, lockFile) {
----------------
sepavloff wrote:
> labath wrote:
> > sepavloff wrote:
> > > labath wrote:
> > > > Have you considered using flock(2) instead of F_SETLK? That might give you semantics which are a bit saner and a bit closer to what happens on windows (though file locking is always weird on posix systems)...
> > > IIUC, `flock` is not a POSIX call. GLIBC implements it on top of `fcntl`. The implementation also contains vague statement that it represents different mechanism on 4BSD: https://github.com/bminor/glibc/blob/92954ffa5a5662fbfde14febd7e5dcc358c85470/sysdeps/posix/flock.c#L18 . So I would refrain from using it, as the code must work on Linux, MacOS and *BSD. POSIX calls looks more portable.
> > You're right that it is not a posix call -- I did not realize that. However, a brief search seems to indicate that all major operating systems (I tried linux, mac, openbsd, freebsd, netbsd) do have this function.
> > 
> > I'm not sure in what situation is the glibc function you linked to used (glibc build system is very opaque to me), but it is definitely not used on linux, as linux kernel has first class support for this via SYS_flock. I'd expect the BSDs (that includes macs) to do the same, as they document flock as behaving differently than fcntl locks.
> > 
> > You're right that fcntl locks are more portable on paper, but I am not really sure that is true in practice. OTOH, I am sure that the fcntl lock semantics are very weird. One example is given in the bsd man pages:
> > ```
> >      ... This semantic means that applica-
> >      tions must be aware of any files that a subroutine library may access.
> >      For example if an application for updating the password file locks the
> >      password file database while making the update, and then calls
> >      getpwnam(3) to retrieve a record, the lock will be lost because
> >      getpwnam(3) opens, reads, and closes the password database.  The database
> >      close will release all locks that the process has associated with the
> >      database, even if the library routine never requested a lock on the data-
> >      base. 
> > ```
> > I also have very bad memories of trying to use this function, because the deadlock detection algorithm used can create false positives in multithreaded applications.
> After some hesitation I implemented your idea to use `flock` instead of `fcntl`. The concern was problems with locking on NFS shares, but it seems this was an issue for old implementation.
> 
> Using `flock` allows to enable unit test for unix as well.
Thanks. We can always revisit this if it turns out to be an issue somewhere.


================
Comment at: llvm/unittests/Support/Path.cpp:1937
 
+TEST_F(FileSystemTest, lockFile) {
+  int FD1, FD2;
----------------
Maybe a test where the second lock is taken on a separate thread, and the first lock is released while the second thread is waiting for it to become available?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896