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

Mon May 11 05:19:56 PDT 2020

krytarowski added inline comments.

================
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) {
----------------
labath wrote:
> 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.
flock() is implemented as a system call on BSDs since 1983, this means that it is pretty universal.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896