Convert ivy lock to use OwnerPrintingPIDLockFile

Review Request #3598 — Created March 23, 2016 and submitted

zundel
pants
zundel/fix-ivy-lock
3075, 3079
80a0429...
pants-reviews
gmalmquist, kwlzn, patricklaw

We've seen some instances of stale locks in CI builders and from developer reports. Use our wrapper around the lockfile library to recover stale locks.

Update lockfile to the latest version (0.12.2)

CI running at https://travis-ci.org/pantsbuild/pants/builds/118070005

The repro case is described in http://github.com/pantsbuild/pants/issues#issue/3075

We already use this kind of lock in the global lock and in the pants daemon.

I tested this manually by:

  • Touching a lockfile to ~/.ivy2/pants/pants_lock_file.lock and then re-running ./pants clean-all compile src/java:: which ran sucessfully.
  • Adding a breakpoint inside of with self._lock:, starting ./pants clean-all compile src/java:: and using kill -9 to kill all pants tasks, then re-running ./pants clean-all compile src/java::
    Verified that the new debug statement for breaking the lock printed and compile worked successfully
  • Ran ./pants clean-all compile src/java:: in one window, then in another window removed the global .pants.run lock and re-ran ./pants clean-all compile src/java::
    Raised exception:
    Exception caught: (<class 'lockfile.AlreadyLocked'>) 
     ...
    Exception message: /Users/zundel/.ivy2/pants/pants_ivy_lock is already locked
  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
  1. 
      
  2. src/python/pants/ivy/ivy.py (Diff revision 1)
     
     

    This suffers from the same issue as our nailgun PID-file used to: the existence of a process with that PID isn't a solid indication that the lock is really still held. I'd suggest that we just write our own flock wrapper--it's not hard.

    1. If you want to address an issue on top of the issue I've found then that's fine. The issue you're now raising has existed for a long time in the global lock and we don't see problems related to it - and if we did, there would be a clear diagnostic message to show us what was wrong. I'm not happy to leave the code in the current state for the next release so let's figure out the best way to address the ivy lock. If that includes fixing this additional bug, then that'd be great.

      I would be happy with:
      - accepting this patch
      - reverting the ivy lock
      - putting it behind a flag and leaving it off by default (with or without this patch)

  3. 
      
  1. lgtm. I do see Patrick's concern here, but afaict that will only be relevant in the event of a stale lock (which should be an exceptional case) - this will at least provide more helpful details on the pid that created the lock than the current failure mode even in the event the lock is mistakenly held due to a reused pid (which is still better than current state where its held regardless, afaict). to Eric's point, if we can follow-on with an improvement to OwnerPrintingPidLockFile to address the stale pid concern then we can fix this issue in all current usages of OwnerPrintingPidLockFile (the pants global lock, the ivy lock and the pantsd launch lock etc) in one fell swoop.

    it's also worth noting that the lockfile library is deprecated (per its entry on pypi) - so we should definitely look at switching to fasteners as part of improving OwnerPrintingPidLockFile.

  2. 
      
  1. Ship It!
  2. 
      
  1. Ship It!
  2. 
      
Review request changed

Status: Closed (submitted)

Change Summary:

Thanks Patrick, Kris, and Garrett! In master at f377d98. There's still some followup work, see: https://github.com/pantsbuild/pants/issues/3075

Loading...