Use a global lock to ensure there are no races in tdir_in_cache

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,6 +391,8 @@ def tdir_in_cache(base):
     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:
@@ -382,6 +404,8 @@ def tdir_in_cache(base):
         lock_data = lock_tdir(tdir)
         atexit.register(remove_tdir, tdir, lock_data)
         return tdir
+    finally:
+        unlock_file(global_lock)
 
 
 tdir_in_cache.scanned = set()

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 <kovid at kovidgoyal.net>
+
+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()

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))
+            time.sleep(0.05)
-        for i in range(5):
+            gl = retry_lock_tdir('t', sleep=0.05)
-            if is_tdir_locked(tdirs[0]):
+            try:
-                break
+                tdirs = list(tdirs_in('t'))
-            time.sleep(1)
+            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():