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

Tue Apr 28 07:29:22 PDT 2020

labath added a reviewer: krytarowski.
labath added a comment.

Looping in Kamil for his knowledge of "obscure" operating systems and other historical trivia.

However, I am still wondering (as I've alluded to in the other review) if there isn't a simpler way to guarantee atomic appends to a "log" file.

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

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896