Add OwnerPrintingInterProcessFileLock and replace OwnerPrintingPIDLockFile.

Review Request #3633 — Created March 31, 2016 and submitted

patricklaw
pants
3124
pants-reviews
benjyw, jsirois, kwlzn, mateor, zundel
* Remove lockfile dependency and use fasteners instead.
* Use a file lock instead of writing out a pid to the lock file.
* Use a separate message file to prevent stomping on the file lock.

CI is green: https://travis-ci.org/pantsbuild/pants/builds/119942027

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
  1. Thanks.

  2. src/python/pants/process/lock.py (Diff revision 1)
     
     

    'Waiting for the file lock at {}...'.format(self.path)

    ?

  3. 
      
  1. lgtm!

  2. 
      
  1. Ship It!
  2. 
      
  1. 
      
  2. src/python/pants/process/lock.py (Diff revision 3)
     
     

    Why print directly to sys.stderr instead of logging?

    1. That's what the old one did, and I was trying to avoid too much thrash in behavior.

  3. 
      
  1. 
      
  2. src/python/pants/process/lock.py (Diff revision 4)
     
     

    nit: dates in both files

  3. I'm a bit leery of using timeouts like this - I think we should be prepared for these tests to flake on a busy CI builder. I like how you use events for lock_held.wait(). Would it work in this test to have the child process wait on an event to tell it that its time for it to release the lock instead of just waiting for 2 seconds to release it?

    1. I changed it to do something like that. I still want it to block on the lock (otherwise it would just get it for free through the non-blocking path, and I want to test the blocking path). Here I made it block for a really long time, but set up a thread to inform the subprocess after a couple seconds that it should release the lock. I think there's still a very slight race and possibility for thrash here, but much less probable than before. And at the very least we're protected from a false positive where we get the lock but didn't go through the blocking path.

  4. 
      
  1. I think this would be more likely to pass in a heavily loaded CI

  2. tests/python/pants_test/process/test_lock.py (Diff revisions 4 - 6)
     
     

    we could still put a timeout in here, just a very long tone. I guess the regular pants test timeouts would 'unstick' this test if it ran into issues?

    1. Probably good for it to have a timeout just so it's impossible for child processes to get leaked, too. I'll add in something big like a minute.

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

Status: Closed (submitted)

Change Summary:

Upstream at 0d78d19ff4a8a44228eb59fac209e0e71e2d9152

Loading...