[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/lib/Support/Windows/Path.inc:1276
+      if (Error == ERROR_LOCK_VIOLATION) {
+        ::Sleep(1);
+        continue;
----------------
amccarth wrote:
> `::Sleep(1)` sleeps for _at least_ one millisecond, but possibly for much longer.  The default tick frequency on Windows is about 16 ms.  (Many apps boost the system's timer frequency to 1 ms, but that's not universal and not recommended except for short periods when an app needs to display real-time media.)
> 
> Sleeping too long once isn't a big deal.  But Counter increments by 1 ms each time through the loop, regardless of how long it actually took, so this is likely to sleep too long many times.  If the user requests a 10 ms timeout, they could actually wait 160 ms (in some near-worst case scenario).
> 
> If this tracked the actual time elapsed, it probably would never be worse than 16 ms.
> 
> You can use chrono's high resolution clock or Windows APIs like ::GetTickCount or ::QueryPerformanceCounter to find out how long the thread actually slept.
Indeed, `Timeout` was in fact a number of attempts. Reworked this place using `std::chrono`.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896