Goals should always release the goal lock after aquiring it.

Review Request #43 — Created March 4, 2014 and submitted

jsirois
commons
jsirois/pants/fix_group_engine
231
benjyw, lahosken, wickman
commit dec968cca44285c395be88ee4ef4e6219f599537
Author: John Sirois <jsirois@twitter.com>
Date:   Mon Mar 3 17:48:35 2014 -0700

    Goals should always release the goal lock after aquiring it.

 src/python/twitter/pants/engine/group_engine.py | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)
Before, things would hang at the second exercise of the GroupEngine in its test since the 1st did not release the lock:
./pants tests/python/twitter/pants/engine/ -v
Build operating on targets: OrderedSet([PythonTests(tests/python/twitter/pants/engine/BUILD:engine)])
============================================================================================================================== test session starts ===============================================================================================================================
platform linux3 -- Python 2.6.8 -- pytest-2.3.5 -- /home/jsirois/.pyenv/versions/2.6.8/bin/python
collected 13 items 

tests/python/twitter/pants/engine/test_group_engine.py:41: GroupMemberTest.test_from_goal_invalid PASSED
tests/python/twitter/pants/engine/test_group_engine.py:34: GroupMemberTest.test_from_goal_valid PASSED
tests/python/twitter/pants/engine/test_group_engine.py:88: GroupIteratorSingleTest.test PASSED
tests/python/twitter/pants/engine/test_group_engine.py:105: GroupIteratorMultipleTest.test PASSED
tests/python/twitter/pants/engine/test_group_engine.py:136: GroupIteratorTargetsTest.test_internal_targets PASSED
tests/python/twitter/pants/engine/test_group_engine.py:140: GroupIteratorTargetsTest.test_non_internal_targets PASSED
tests/python/twitter/pants/engine/test_group_engine.py:220: GroupEngineTest.test_groups PASSED
tests/python/twitter/pants/engine/test_group_engine.py:198: GroupEngineTest.test_no_groups ^CProblem executing PythonBuilder for targets OrderedSet([PythonTests(tests/python/twitter/pants/engine/BUILD:engine)]): Traceback (most recent call last):



After, all is good:
./pants tests/python/twitter/pants/engine/ -v
Build operating on targets: OrderedSet([PythonTests(tests/python/twitter/pants/engine/BUILD:engine)])
============================================================================================================================== test session starts ===============================================================================================================================
platform linux3 -- Python 2.6.8 -- pytest-2.3.5 -- /home/jsirois/.pyenv/versions/2.6.8/bin/python
collected 13 items 

tests/python/twitter/pants/engine/test_group_engine.py:41: GroupMemberTest.test_from_goal_invalid PASSED
tests/python/twitter/pants/engine/test_group_engine.py:34: GroupMemberTest.test_from_goal_valid PASSED
tests/python/twitter/pants/engine/test_group_engine.py:88: GroupIteratorSingleTest.test PASSED
tests/python/twitter/pants/engine/test_group_engine.py:105: GroupIteratorMultipleTest.test PASSED
tests/python/twitter/pants/engine/test_group_engine.py:136: GroupIteratorTargetsTest.test_internal_targets PASSED
tests/python/twitter/pants/engine/test_group_engine.py:140: GroupIteratorTargetsTest.test_non_internal_targets PASSED
tests/python/twitter/pants/engine/test_group_engine.py:220: GroupEngineTest.test_groups PASSED
tests/python/twitter/pants/engine/test_group_engine.py:198: GroupEngineTest.test_no_groups PASSED
tests/python/twitter/pants/engine/test_engine.py <- ../../../../../tmp/tmpANmXwr/.deps/mox-0.5.3-py2.6.egg/mox.py:1858: TimerTest.test_begin PASSED
tests/python/twitter/pants/engine/test_engine.py:76: ExecutionOrderTest.test_execution_order PASSED
tests/python/twitter/pants/engine/test_engine.py:136: EngineTest.test_execute_code PASSED
tests/python/twitter/pants/engine/test_engine.py:130: EngineTest.test_execute_raise PASSED
tests/python/twitter/pants/engine/test_engine.py:119: EngineTest.test_execute_success PASSED

=========================================================================================================================== 13 passed in 0.30 seconds ============================================================================================================================
tests.python.twitter.pants.engine.engine                                        .....   SUCCESS
  • 0
  • 0
  • 0
  • 1
  • 1
Description From Last Updated
JS
  1. Still hunting around for the old behavior.  This does not overlap locks which may have been done, it just serializes per goal period such that competing pants processes may alternate as lock holders.
  2. 
      
WI
  1. 
      
  2. shouldn't we just change Context.acquire_lock/release_lock to be a contextmanager?
    1. My investigation of old behavior does show it supported overlaps [1] - taking another spin on this.
      
      [1] https://github.com/twitter/commons/blob/cf0e820aa606b198dedfa38a50413947d7e48af5/src/python/twitter/pants/goal/group.py
  3. 
      
BE
  1. 
      
  2. Name this "lock_if_necessary" or something? Makes the code more readable. 
    1. Totally re-working this based on last-known-pre-merge-good form here: https://github.com/twitter/commons/blob/677780ea56af610359b3ccb30accb5b9ecd713d8/src/python/twitter/pants/goal/group.py
  3. 
      
JS
WI
  1. 
      
  2. is it safe to call release_lock twice?  because i believe it's possible to do it here, right?
    1. It definitely will get called 2x in the normal serilized case and it is ok:
        def release_lock(self):
          """Release the global lock if it's held.
          Returns True if the lock was held before this call.
          """
          if self._lock.is_unlocked():
            return False
          else:
            self._lock.release()
            self._lock = Lock.unlocked()
            return True
  3. 
      
JS
WI
  1. Ship It!
  2. 
      
JS
JS
  1. Thanks, submitted @ https://github.com/twitter/commons/commit/5b2ecdccd2e4f185c1ecbd2dedba621d5b41ad4b
  2. 
      
JS
Review request changed

Status: Closed (submitted)

Loading...