[checkmk-commits] store unlock: Use same logic for release_all_locks() and release_lock()

Module: check_mk Branch: master Commit: 5dea7b935516a97d8e385dff69edfafed458b92e URL: http://git.mathias-kettner.de/git/?p=check_mk.git;a=commit;h=5dea7b935516a9… Author: Lars Michelsen <lm(a)mathias-kettner.de> Date: Sun Oct 7 11:50:32 2018 +0200 store unlock: Use same logic for release_all_locks() and release_lock() The previous implementation of release_lock() could fail in case the file handle was closed before calling release_lock() because release_lock() did not ignore already closed files. Change-Id: Ib6de4e1e4e57d2437051ca60d9bd4486a0033034 --- cmk/store.py | 25 ++++++++++++++----------- tests/unit/cmk/test_store.py | 31 +++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 11 deletions(-) diff --git a/cmk/store.py b/cmk/store.py index 3bcdf86..fbc3fad 100644 --- a/cmk/store.py +++ b/cmk/store.py @@ -315,10 +315,19 @@ def release_lock(path): return # no unlocking needed for lock_path, fd in g_aquired_locks: - if lock_path == path: - fcntl.flock(fd, fcntl.LOCK_UN) + if lock_path != path: + continue + + try: os.close(fd) - g_aquired_locks.remove((lock_path, fd)) + except OSError as e: + if e.errno == 9: # OSError: [Errno 9] Bad file descriptor + pass + else: + raise + + g_aquired_locks.remove((lock_path, fd)) + break g_locked_paths.remove(path) @@ -330,14 +339,8 @@ def have_lock(path): def release_all_locks(): global g_aquired_locks, g_locked_paths - for _unused_path, fd in g_aquired_locks: - try: - os.close(fd) - except OSError, e: - if e.errno == 9: # OSError: [Errno 9] Bad file descriptor - pass - else: - raise + for path, _unused_fd in g_aquired_locks: + release_lock(path) g_aquired_locks = [] g_locked_paths = [] diff --git a/tests/unit/cmk/test_store.py b/tests/unit/cmk/test_store.py index e577e29..cd67da8 100644 --- a/tests/unit/cmk/test_store.py +++ b/tests/unit/cmk/test_store.py @@ -150,6 +150,22 @@ def test_release_lock(tmpdir): assert store.have_lock(path) == False +def test_release_lock_already_closed(tmpdir): + locked_file = tmpdir.join("locked_file") + locked_file.write("") + + path = "%s" % locked_file + + assert store.have_lock(path) == False + store.aquire_lock(path) + assert store.have_lock(path) == True + + os.close([ fd for p, fd in store.g_aquired_locks if p == path ][0]) + + store.release_lock(path) + assert store.have_lock(path) == False + + def test_release_all_locks(tmpdir): locked_file1 = tmpdir.join("locked_file1") locked_file1.write("") @@ -172,6 +188,21 @@ def test_release_all_locks(tmpdir): assert store.have_lock(path2) == False +def test_release_all_locks_already_closed(tmpdir): + locked_file = tmpdir.join("locked_file") + locked_file.write("") + + path = "%s" % locked_file + + assert store.have_lock(path) == False + store.aquire_lock(path) + assert store.have_lock(path) == True + + os.close([ fd for p, fd in store.g_aquired_locks if p == path ][0]) + + store.release_all_locks() + assert store.have_lock(path) == False + class LockTestThread(threading.Thread): def __init__(self, store, path):