From 196cb751d3645c6cf63d222265d4718d2c7d8da1 Mon Sep 17 00:00:00 2001 From: Kovid Goyal Date: Sun, 28 Jan 2018 08:44:18 +0530 Subject: [PATCH] Use a global lock to ensure there are no races in tdir_in_cache --- src/calibre/ptempfile.py | 46 +++++++--- src/calibre/utils/tdir_in_cache.py | 129 +++++++++++++++++++++++++++++ src/calibre/utils/test_lock.py | 23 +++-- 3 files changed, 178 insertions(+), 20 deletions(-) create mode 100644 src/calibre/utils/tdir_in_cache.py diff --git a/src/calibre/ptempfile.py b/src/calibre/ptempfile.py index 75bd4db4f1..1f31a9336a 100644 --- a/src/calibre/ptempfile.py +++ b/src/calibre/ptempfile.py @@ -301,6 +301,9 @@ if iswindows: from calibre.utils.lock import windows_open return windows_open(os.path.join(path, TDIR_LOCK)) + def unlock_file(fobj): + fobj.close() + def remove_tdir(path, lock_file): lock_file.close() remove_dir(path) @@ -323,6 +326,11 @@ else: eintr_retry_call(fcntl.lockf, f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) return f + def unlock_file(fobj): + from calibre.utils.ipc import eintr_retry_call + eintr_retry_call(fcntl.lockf, fobj.fileno(), fcntl.LOCK_UN) + fobj.close() + def remove_tdir(path, lock_file): lock_file.close() remove_dir(path) @@ -361,6 +369,18 @@ def clean_tdirs_in(b): remove_dir(q) +def retry_lock_tdir(path, timeout=30): + from calibre.utils.monotonic import monotonic + st = monotonic() + while True: + try: + return lock_tdir(path) + except Exception: + if monotonic() - st > timeout: + raise + time.sleep(0.1) + + def tdir_in_cache(base): ''' Create a temp dir inside cache_dir/base. The created dir is robust against application crashes. i.e. it will be cleaned up the next time the @@ -371,17 +391,21 @@ def tdir_in_cache(base): except EnvironmentError as e: if e.errno != errno.EEXIST: raise - if b not in tdir_in_cache.scanned: - tdir_in_cache.scanned.add(b) - try: - clean_tdirs_in(b) - except Exception: - import traceback - traceback.print_exc() - tdir = _make_dir('', '', b) - lock_data = lock_tdir(tdir) - atexit.register(remove_tdir, tdir, lock_data) - return tdir + global_lock = retry_lock_tdir(b) + try: + if b not in tdir_in_cache.scanned: + tdir_in_cache.scanned.add(b) + try: + clean_tdirs_in(b) + except Exception: + import traceback + traceback.print_exc() + tdir = _make_dir('', '', b) + lock_data = lock_tdir(tdir) + atexit.register(remove_tdir, tdir, lock_data) + return tdir + finally: + unlock_file(global_lock) tdir_in_cache.scanned = set() diff --git a/src/calibre/utils/tdir_in_cache.py b/src/calibre/utils/tdir_in_cache.py new file mode 100644 index 0000000000..2c97c49d28 --- /dev/null +++ b/src/calibre/utils/tdir_in_cache.py @@ -0,0 +1,129 @@ +#!/usr/bin/env python2 +# vim:fileencoding=utf-8 +# License: GPLv3 Copyright: 2018, Kovid Goyal + +from __future__ import absolute_import, division, print_function, unicode_literals + +import atexit +import errno +import os +import tempfile +import time + +from calibre.constants import cache_dir, iswindows +from calibre.ptempfile import remove_dir +from calibre.utils.monotonic import monotonic + +TDIR_LOCK = u'tdir-lock' + +if iswindows: + from calibre.utils.lock import windows_open + + def lock_tdir(path): + return windows_open(os.path.join(path, TDIR_LOCK)) + + def unlock_file(fobj): + fobj.close() + + def remove_tdir(path, lock_file): + lock_file.close() + remove_dir(path) + + def is_tdir_locked(path): + try: + with windows_open(os.path.join(path, TDIR_LOCK)): + pass + except EnvironmentError: + return True + return False +else: + import fcntl + from calibre.utils.ipc import eintr_retry_call + + def lock_tdir(path): + lf = os.path.join(path, TDIR_LOCK) + f = lopen(lf, 'w') + eintr_retry_call(fcntl.lockf, f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + return f + + def unlock_file(fobj): + from calibre.utils.ipc import eintr_retry_call + eintr_retry_call(fcntl.lockf, fobj.fileno(), fcntl.LOCK_UN) + fobj.close() + + def remove_tdir(path, lock_file): + lock_file.close() + remove_dir(path) + + def is_tdir_locked(path): + lf = os.path.join(path, TDIR_LOCK) + f = lopen(lf, 'w') + try: + eintr_retry_call(fcntl.lockf, f.fileno(), fcntl.LOCK_EX | fcntl.LOCK_NB) + eintr_retry_call(fcntl.lockf, f.fileno(), fcntl.LOCK_UN) + return False + except EnvironmentError: + return True + finally: + f.close() + + +def tdirs_in(b): + try: + tdirs = os.listdir(b) + except EnvironmentError as e: + if e.errno != errno.ENOENT: + raise + tdirs = () + for x in tdirs: + x = os.path.join(b, x) + if os.path.isdir(x): + yield x + + +def clean_tdirs_in(b): + # Remove any stale tdirs left by previous program crashes + for q in tdirs_in(b): + if not is_tdir_locked(q): + remove_dir(q) + + +def retry_lock_tdir(path, timeout=30, sleep=0.1): + st = monotonic() + while True: + try: + return lock_tdir(path) + except Exception: + if monotonic() - st > timeout: + raise + time.sleep(sleep) + + +def tdir_in_cache(base): + ''' Create a temp dir inside cache_dir/base. The created dir is robust + against application crashes. i.e. it will be cleaned up the next time the + application starts, even if it was left behind by a previous crash. ''' + b = os.path.join(cache_dir(), base) + try: + os.makedirs(b) + except EnvironmentError as e: + if e.errno != errno.EEXIST: + raise + global_lock = retry_lock_tdir(b) + try: + if b not in tdir_in_cache.scanned: + tdir_in_cache.scanned.add(b) + try: + clean_tdirs_in(b) + except Exception: + import traceback + traceback.print_exc() + tdir = tempfile.mkdtemp(dir=b) + lock_data = lock_tdir(tdir) + atexit.register(remove_tdir, tdir, lock_data) + return tdir + finally: + unlock_file(global_lock) + + +tdir_in_cache.scanned = set() diff --git a/src/calibre/utils/test_lock.py b/src/calibre/utils/test_lock.py index 0bf23e2428..e1aacc7043 100644 --- a/src/calibre/utils/test_lock.py +++ b/src/calibre/utils/test_lock.py @@ -14,8 +14,11 @@ import unittest from threading import Thread from calibre.constants import cache_dir, fcntl, iswindows -from calibre.ptempfile import clean_tdirs_in, is_tdir_locked, tdir_in_cache, tdirs_in from calibre.utils.lock import ExclusiveFile, create_single_instance_mutex, unix_open +from calibre.utils.tdir_in_cache import ( + clean_tdirs_in, is_tdir_locked, retry_lock_tdir, tdir_in_cache, tdirs_in, + unlock_file +) def FastFailEF(name): @@ -135,21 +138,22 @@ class IPCLockTest(unittest.TestCase): child = run_worker('calibre.utils.test_lock', 'other4') tdirs = [] while not tdirs: - tdirs = list(tdirs_in('t', check_for_lock=True)) - for i in range(5): - if is_tdir_locked(tdirs[0]): - break - time.sleep(1) + time.sleep(0.05) + gl = retry_lock_tdir('t', sleep=0.05) + try: + tdirs = list(tdirs_in('t')) + finally: + unlock_file(gl) self.assertTrue(is_tdir_locked(tdirs[0])) c2 = run_worker('calibre.utils.test_lock', 'other5') - c2.wait() + self.assertEqual(c2.wait(), 0) self.assertTrue(is_tdir_locked(tdirs[0])) child.kill(), child.wait() self.assertTrue(os.path.exists(tdirs[0])) self.assertFalse(is_tdir_locked(tdirs[0])) clean_tdirs_in('t') self.assertFalse(os.path.exists(tdirs[0])) - self.assertFalse(os.listdir('t')) + self.assertEqual(os.listdir('t'), [u'tdir-lock']) def other1(): @@ -179,7 +183,8 @@ def other4(): def other5(): cache_dir.ans = os.getcwdu() - tdir_in_cache('t') + if not os.path.isdir(tdir_in_cache('t')): + raise SystemExit(1) def find_tests():