python · rhettinger · Feb 2, 2019 · Feb 2, 2019 · Feb 2, 2019 · Feb 2, 2019
@@ -661,6 +661,25 @@ sequence is empty.");
 
 /* lru_cache object **********************************************************/
 
+/* There are four principal algorithmic differences from the pure python version:
+
+   1). The C version relies on the GIL instead of having its own reentrant lock.
+
+   2). The prev/next link fields use borrowed references.
+
+   3). For a full cache, the pure python version rotates the location of the
+       root entry so that it never has to move individual links and it can
+       limit updates to just the key and result fields.  However, in the C
+       version, links are temporarily removed while the cache dict updates are
+       occurring. Afterwards, they are appended or prepended back into the
+       doubly-linked lists.
+
+   4)  In the Python version, a _HashSeq class is used to prevent __hash__
+       from being called more than once.  In the C version, the "known hash"
+       variants of dictionary calls as used to the same effect.
+
+*/
+
 /* this object is used delimit args and keywords in the cache keys */
 static PyObject *kwd_mark = NULL;
 
@@ -844,33 +863,52 @@ infinite_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwd
     return result;
 }
 
+/* Mark a link as unused when it is created or when it is extracted
+   from the doubly-linked list.  Use this marker to avoid being
+   updated by two threads at the same time.  We don't want to
+   append a link unless we know it is not already in used
+   and we don't want to extract a link that is NULL or already
+   in the process of being extracted.
+ */
+#define MARK_UNUSED(link)  {link->prev = NULL; link->next = NULL;}
+#define IS_USED(link) (link != NULL && link->prev != NULL && link->next != NULL)
+
 static void
 lru_cache_extract_link(lru_list_elem *link)
 {
-    lru_list_elem *link_prev = link->prev;
-    lru_list_elem *link_next = link->next;
+    lru_list_elem *link_prev, *link_next;
+    assert(IS_USED(link));
+    link_prev = link->prev;
+    link_next = link->next;
     link_prev->next = link->next;
     link_next->prev = link->prev;
+    MARK_UNUSED(link);
 }
 
 static void
 lru_cache_append_link(lru_cache_object *self, lru_list_elem *link)
 {
-    lru_list_elem *root = &self->root;
-    lru_list_elem *last = root->prev;
+    lru_list_elem *root, *last;
+    assert(!IS_USED(link));
+    root = &self->root;
+    last = root->prev;
     last->next = root->prev = link;
     link->prev = last;
     link->next = root;
+    assert(IS_USED(link));
 }
 
 static void
 lru_cache_prepend_link(lru_cache_object *self, lru_list_elem *link)
 {
-    lru_list_elem *root = &self->root;
-    lru_list_elem *first = root->next;
+    lru_list_elem *root, *first;
+    assert(!IS_USED(link));
+    root = &self->root;
+    first = root->next;
     first->prev = root->next = link;
     link->prev = root;
     link->next = first;
+    assert(IS_USED(link));
 }
 
 /* General note on reentrancy:
@@ -920,7 +958,12 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
         return NULL;
     }
     link  = (lru_list_elem *)_PyDict_GetItem_KnownHash(self->cache, key, hash);
-    if (link != NULL) {
+    if (link != NULL && IS_USED(link)) {
+        /* Either the __hash__ or __eq__ call can let another thread
+           also get to this same link, so we need to move the link
+           atomically (extract and append with in one-step guarded by
+           the GIL).  The only works if some other thread has not
+           already detached the link. */
         lru_cache_extract_link(link);
         lru_cache_append_link(self, link);
         result = link->result;
@@ -939,6 +982,10 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
         Py_DECREF(key);
         return NULL;
     }
+    /* We now hold references to a key and a result. The hash value is known.
+       After the callback to the user function, we no longer know anything
+       about the contents of the cache, so we have to check to see whether
+       an equivalent key has already been added. */
     testresult = _PyDict_GetItem_KnownHash(self->cache, key, hash);
     if (testresult != NULL) {
         /* Getting here means that this same key was added to the cache
@@ -948,18 +995,24 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
         return result;
     }
     if (PyErr_Occurred()) {
-        /* This is an unusual case since this same lookup
-           did not previously trigger an error during lookup.
-           Treat it the same as an error in user function
-           and return with the error set. */
+        /* This is an unusual case since this same lookup did not
+           previously trigger an error during lookup.  Treat it the same
+           as an error in user function and return with the error
+           set. */
         Py_DECREF(key);
         Py_DECREF(result);
         return NULL;
     }
-    /* This is the normal case.  The new key wasn't found before
-       user function call and it is still not there.  So we
-       proceed normally and update the cache with the new result. */
-
+    /* This is the normal case.  The new key wasn't found before the user
+       function call and it is still not there.  So we proceed normally
+       and update the cache with the new result.
+
+       The dict lookup can trigger arbitrary code if there is an __eq__
+       call (this only occurs when the full hash value exactly matches
+       that for another key).  That means that we don't really know
+       the key is still missing.  XXX Could state flag for the cache
+       detect this exotic event?
+    */
     assert(self->maxsize > 0);
     if (PyDict_GET_SIZE(self->cache) < self->maxsize ||
         self->root.next == &self->root)
@@ -972,22 +1025,36 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
             Py_DECREF(result);
             return NULL;
         }
-
-        link->hash = hash;
-        link->key = key;
-        link->result = result;
         /* What is really needed here is a SetItem variant with a "no clobber"
            option.  If the __eq__ call triggers a reentrant call that adds
-           this same key, then this setitem call will update the cache dict
+           this same key, then this SetItem call will update the cache dict
            with this new link, leaving the old link as an orphan (i.e. not
-           having a cache dict entry that refers to it). */
+           having a cache dict entry that refers to it).
+
+           The link is marked as unused because the don't want to add it
+           to the doubly-linked list until we know that updating the
+           cache dict was successful.  Marking it as unused prevents
+           other threads from using this link before all of its fields
+           are valid.
+        */
+        MARK_UNUSED(link);
         if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link,
                                       hash) < 0) {
             Py_DECREF(link);
             return NULL;
         }
-        lru_cache_append_link(self, link);
+        if (IS_USED(link)) {
+            /* XXX If we can prove this case can't happen, turn this test
+               into an assertion. */
+            Py_DECREF(key);
+            Py_DECREF(link);
+            return result;
+        }
         Py_INCREF(result); /* for return */
+        link->hash = hash;
+        link->key = key;
+        link->result = result;
+        lru_cache_append_link(self, link);
         return result;
     }
     /* Since the cache is full, we need to evict an old key and add
@@ -1004,13 +1071,25 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
     */
     PyObject *oldkey, *oldresult, *popresult;
 
-    /* Extract the oldest item. */
+    /* Get the oldest link. */
     assert(self->root.next != &self->root);
     link = self->root.next;
+    if (!IS_USED(link)) {
+        /* Getting here means another thread is in the process of
+           removing this link, so let that proceed normally and
+           let's just return the calculated result.*/
+        Py_DECREF(key);
+        return result;
+    }
+    /* Extract the oldest item. */
     lru_cache_extract_link(link);
     /* Remove it from the cache.
        The cache dict holds one reference to the link,
-       and the linked list holds yet one reference to it. */
+       and the linked list holds yet one reference to it.
+
+       XXX Double check this statement.  The linked list
+       is documented above to only have borrowed references.
+    */
     popresult = _PyDict_Pop_KnownHash(self->cache, link->key,
                                       link->hash, Py_None);
     if (popresult == Py_None) {
@@ -1029,7 +1108,12 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
            original position as the oldest link.  Then we allow the
            error propagate upward; treating it the same as an error
            arising in the user function. */
-        lru_cache_prepend_link(self, link);
+        if (!IS_USED(link)) {
+            lru_cache_prepend_link(self, link);
+        }
+        else {
+            Py_DECREF(link);
+        }
         Py_DECREF(key);
         Py_DECREF(result);
         return NULL;
@@ -1048,7 +1132,7 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
        prev and next fields set to valid values.   We have to wait
        for successful insertion in the cache dict before adding the
        link to the linked list.  Otherwise, the potentially reentrant
-       __eq__ call could cause the then ophan link to be visited. */
+       __eq__ call could cause the then orphan link to be visited. */
     if (_PyDict_SetItem_KnownHash(self->cache, key, (PyObject *)link,
                                   hash) < 0) {
         /* Somehow the cache dict update failed.  We no longer can
@@ -1060,8 +1144,13 @@ bounded_lru_cache_wrapper(lru_cache_object *self, PyObject *args, PyObject *kwds
         Py_DECREF(oldresult);
         return NULL;
     }
-    lru_cache_append_link(self, link);
     Py_INCREF(result); /* for return */
+    if (!IS_USED(link)) {
+        lru_cache_append_link(self, link);
+    }
+    else {
+        Py_DECREF(link);
+    }
     Py_DECREF(popresult);
     Py_DECREF(oldkey);
     Py_DECREF(oldresult);