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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
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);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78896