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

[Serge Pavlov via Phabricator via llvm-commits]

sepavloff marked an inline comment as done.
sepavloff added a comment.

In D78896#2079169 <https://reviews.llvm.org/D78896#2079169>, @labath wrote:

> It's a pitty about flock&solaris. It's a much saner api, and a lot closer to the windows implementation.
>
> I guess we have to stick to fcntl then. The new version looks good. I just think it would be good to acknowledge the weirdness of the flock api in the method documentation. Maybe something like "Care should be taken when using this function in a multithreaded context, as it may not prevent other threads in the same process from obtaining a lock on the same file, even if they are using a different file descriptor."


Added the note in the description of the function in FileSystem.h.



================
Comment at: llvm/lib/Support/Unix/Path.inc:1066
+      return std::error_code(Error, std::generic_category());
+    usleep(1000);
+  } while (std::chrono::steady_clock::now() < End);
----------------
MaskRay wrote:
> labath wrote:
> > sepavloff wrote:
> > > MaskRay wrote:
> > > > labath wrote:
> > > > > MaskRay wrote:
> > > > > > I feel uneasy with usleep(1000). Why is it needed?
> > > > > That's because the os provides no mechanism to wait for a lock to become available for a given amount of time (well.... one could use `SIGALRM`s to achieve that, but those come with problems of their own..)
> > > > > 
> > > > > Using exponential backoff for this might be a good idea, but I also think that can wait until this becomes a problem.
> > > > The standard `try_*` (std::mutex::try_lock, pthread_mutex_trylock, etc) return immediately upon a failure. The backoff strategy should be done by the application to avoid the misnomer.
> > > If the function is called as `tryLockFile(FD)` it behaves exactly as the standard `try_*` functions, it returns immediately.
> > > 
> > > > The backoff strategy should be done by the application to avoid the misnomer.
> > > 
> > > This use case is often and even standard library provides functions that attempt to do an operation in specified time (*_for). As the only expected usage of file locks is writing to log file by concurrent processes and it requires repeating the operation if unsuccessful, so integrations of the wait loop into the service function seems natural.
> > @MaskRay, I'm not sure how to interpret your comment -- would your concerns be addressed if this function was renamed to `tryLockFileFor` (and deleting the default argument value) ? Because I too think that would be a better name for this function (though I don't feel very strongly about that). `tryLockFile` could then be implemented as `tryLockFileFor(seconds(0))`, or not implemented at all, if it's not needed.
> > 
> > As for the retries, I agree with @sepavloff, that it makes sense to implement them here. The way I see it the "retries" are an implementation detail and the function as a whole still behaves like the other `try_lock_for` functions. E.g. description of `std::timed_mutex::try_lock_for` says: "tries to lock the mutex, returns if the mutex has been unavailable for the specified timeout duration" -- that is still true. The main difference is that the function returns an error_code instead of a bool, but there's not much that can be done about that, as this operation can fail for a lot of other reasons.
> > 
> > I think this is sort similar to `std::atomic<T>::fetch_add`. E.g., arm64 does not have the equivalent of the `lock add` instruction, so this function compiles to a `ldaxr+stlxr` loop, which attempts the update several times until successful -- the caller does not have to retry manually. (And down at the microarchitectural level, even `lock add` probably uses retries&timeouts to implement the functionality.)
> Adding both `try` and `for` to the name looks good to me.
> 
> POSIX uses `timedlock` which may be considered as well.

Using `for` is convenient for standard functions, because they accept single argument of type `std::chrono::duration`, so an expression like:
```
    f.wait_for(2500ms)
```
looks self-documented. In the case of `tryLockFile` it becomes something like:
```
    tryLockFileFor(FileHandle, 2500ms)
```
which is pretty ugly.

I renamed `tryLockFile` to `tryLockFileWithTimeout`, which must be unambiguous, although long. Also a function `tryLock` was added, that does not wait and return immediately.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896