Do not stall if xactions overwrite a recently active cache entry (#516)

After the last transaction that cached or read the reply R1 ended, its
Transients entry T1 was not freed. Subsequent requests (with different
public keys) could occupy the same shared memory and rock slots (purging
unlocked R1), preventing Squid from attaching a T1-derived StoreEntry to
the cache. Another request for R1 would receive T1 and stall because its
worker W1 kept waiting for a notification from another worker W2,
incorrectly assuming that W2 exists and is going to fetch R1 for W1.
That request was aborted after a timeout.

A Transients entry represents active transaction(s). Broadcasts stop
when there are no transactions to inform. We must remove idle (i.e.
unlocked) Transients entries to avoid feeding new transactions with
stale info. We now do that when unlocking a Transients entry and also
double check that a found unattached Transients entry has a writer.

diff --git a/src/Transients.cc b/src/Transients.cc
index 5e434b2a548d082cc03c31125637aae06ac51734..652cdc1dd43537eb6564973747b3a4cee71b5969 100644 (file)
--- a/src/Transients.cc
+++ b/src/Transients.cc
@@ -157,7 +157,7 @@ Transients::get(const cache_key *key)
     if (StoreEntry *oldE = locals->at(index)) {
         debugs(20, 3, "not joining private " << *oldE);
         assert(EBIT_TEST(oldE->flags, KEY_PRIVATE));
-        map->closeForReading(index);
+        map->closeForReadingAndFreeIdle(index);
         return nullptr;
     }
 
@@ -251,6 +251,14 @@ Transients::addEntry(StoreEntry *e, const cache_key *key, const Store::IoStatus
     }
 }
 
+bool
+Transients::hasWriter(const StoreEntry &e)
+{
+    if (!e.hasTransients())
+        return false;
+    return map->peekAtWriter(e.mem_obj->xitTable.index);
+}
+
 void
 Transients::noteFreeMapSlice(const Ipc::StoreMapSliceId)
 {
@@ -325,7 +333,7 @@ Transients::disconnect(StoreEntry &entry)
             map->abortWriting(xitTable.index);
         } else {
             assert(isReader(entry));
-            map->closeForReading(xitTable.index);
+            map->closeForReadingAndFreeIdle(xitTable.index);
         }
         locals->at(xitTable.index) = nullptr;
         xitTable.index = -1;

diff --git a/src/Transients.h b/src/Transients.h
index b24899142496279e3daf1fcfb9bafa4e2af9ca85..8d2c0280ea3c4749217abc3642e44df5a5560112 100644 (file)
--- a/src/Transients.h
+++ b/src/Transients.h
@@ -85,6 +85,8 @@ public:
     bool isReader(const StoreEntry &) const;
     /// whether the entry is in "writing to Transients" I/O state
     bool isWriter(const StoreEntry &) const;
+    /// whether we or somebody else is in the "writing to Transients" I/O state
+    bool hasWriter(const StoreEntry &);
 
     static int64_t EntryLimit();
 

diff --git a/src/ipc/ReadWriteLock.cc b/src/ipc/ReadWriteLock.cc
index 49f2d281d45b35a698bfc638426839f47deabe49..dbeabad69c50283ef68e63fe3477db519dde008d 100644 (file)
--- a/src/ipc/ReadWriteLock.cc
+++ b/src/ipc/ReadWriteLock.cc
@@ -91,6 +91,26 @@ Ipc::ReadWriteLock::switchExclusiveToShared()
     unlockExclusive();
 }
 
+bool
+Ipc::ReadWriteLock::unlockSharedAndSwitchToExclusive()
+{
+    assert(readers > 0);
+    if (!writeLevel++) { // we are the first writer + lock "new" readers out
+        assert(!appending);
+        unlockShared();
+        if (!readers) {
+            writing = true;
+            return true;
+        }
+        // somebody is still reading: fall through
+    } else {
+        // somebody is still writing: just stop reading
+        unlockShared();
+    }
+    --writeLevel;
+    return false;
+}
+
 void
 Ipc::ReadWriteLock::startAppending()
 {

diff --git a/src/ipc/ReadWriteLock.h b/src/ipc/ReadWriteLock.h
index 753552e817b768b414804415b36d9df1719b2144..75cddee3e87b4ee46a0d2c238eb7612ccd977fb4 100644 (file)
--- a/src/ipc/ReadWriteLock.h
+++ b/src/ipc/ReadWriteLock.h
@@ -35,6 +35,9 @@ public:
     void unlockExclusive(); ///< undo successful exclusiveLock()
     void unlockHeaders(); ///< undo successful lockHeaders()
     void switchExclusiveToShared(); ///< stop writing, start reading
+    /// same as unlockShared() but also attempts to get a writer lock beforehand
+    /// \returns whether the writer lock was acquired
+    bool unlockSharedAndSwitchToExclusive();
 
     void startAppending(); ///< writer keeps its lock but also allows reading
 

diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index f3feb1d633525b8cb9891fabcf303650566e122d..184ece55103e445aabd27d5bc47737de331c559f 100644 (file)
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -247,10 +247,18 @@ Ipc::StoreMap::peekAtReader(const sfileno fileno) const
     const Anchor &s = anchorAt(fileno);
     if (s.reading())
         return &s; // immediate access by lock holder so no locking
+    assert(s.writing()); // must be locked for reading or writing
+    return nullptr;
+}
+
+const Ipc::StoreMap::Anchor *
+Ipc::StoreMap::peekAtWriter(const sfileno fileno) const
+{
+    const Anchor &s = anchorAt(fileno);
     if (s.writing())
-        return NULL; // the caller is not a read lock holder
-    assert(false); // must be locked for reading or writing
-    return NULL;
+        return &s; // immediate access by lock holder so no locking
+    assert(s.reading()); // must be locked for reading or writing
+    return nullptr;
 }
 
 const Ipc::StoreMap::Anchor &
@@ -442,6 +450,23 @@ Ipc::StoreMap::closeForReading(const sfileno fileno)
     debugs(54, 5, "closed entry " << fileno << " for reading " << path);
 }
 
+void
+Ipc::StoreMap::closeForReadingAndFreeIdle(const sfileno fileno)
+{
+    auto &s = anchorAt(fileno);
+    assert(s.reading());
+
+    if (!s.lock.unlockSharedAndSwitchToExclusive()) {
+        debugs(54, 5, "closed entry " << fileno << " for reading " << path);
+        return;
+    }
+
+    assert(s.writing());
+    assert(!s.reading());
+    freeChain(fileno, s, false);
+    debugs(54, 5, "closed idle entry " << fileno << " for reading " << path);
+}
+
 bool
 Ipc::StoreMap::openForUpdating(Update &update, const sfileno fileNoHint)
 {

diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h
index 402165d7b753f9b8a7fa523e5c7bfcec53522b35..c4224f28429f914270e9f2afcf8020d7cf3fb30e 100644 (file)
--- a/src/ipc/StoreMap.h
+++ b/src/ipc/StoreMap.h
@@ -259,10 +259,16 @@ public:
     /// undoes partial update, unlocks, and cleans up
     void abortUpdating(Update &update);
 
-    /// only works on locked entries; returns nil unless the slice is readable
+    /// the caller must hold a lock on the entry
+    /// \returns nullptr unless the slice is readable
     const Anchor *peekAtReader(const sfileno fileno) const;
 
-    /// only works on locked entries; returns the corresponding Anchor
+    /// the caller must hold a lock on the entry
+    /// \returns nullptr unless the slice is writeable
+    const Anchor *peekAtWriter(const sfileno fileno) const;
+
+    /// the caller must hold a lock on the entry
+    /// \returns the corresponding Anchor
     const Anchor &peekAtEntry(const sfileno fileno) const;
 
     /// free the entry if possible or mark it as waiting to be freed if not
@@ -285,6 +291,8 @@ public:
     const Anchor *openForReadingAt(const sfileno fileno);
     /// closes open entry after reading, decrements read level
     void closeForReading(const sfileno fileno);
+    /// same as closeForReading() but also frees the entry if it is unlocked
+    void closeForReadingAndFreeIdle(const sfileno fileno);
 
     /// writeable slice within an entry chain created by openForWriting()
     Slice &writeableSlice(const AnchorId anchorId, const SliceId sliceId);

diff --git a/src/store/Controller.cc b/src/store/Controller.cc
index c43c9eca1541c1a7c0657975ce770a6a8c66ddc5..ef18b8a85ebff7623122c53657c08d0e51ccb7cb 100644 (file)
--- a/src/store/Controller.cc
+++ b/src/store/Controller.cc
@@ -334,7 +334,7 @@ Store::Controller::find(const cache_key *key)
             return entry;
         } catch (const std::exception &ex) {
             debugs(20, 2, "failed with " << *entry << ": " << ex.what());
-            entry->release(true);
+            entry->release();
             // fall through
         }
     }
@@ -355,6 +355,18 @@ Store::Controller::allowSharing(StoreEntry &entry, const cache_key *key)
         const bool found = anchorToCache(entry, inSync);
         if (found && !inSync)
             throw TexcHere("cannot sync");
+        if (!found) {
+            // !found should imply hittingRequiresCollapsing() regardless of writer presence
+            if (!entry.hittingRequiresCollapsing()) {
+                debugs(20, DBG_IMPORTANT, "BUG: missing ENTRY_REQUIRES_COLLAPSING for " << entry);
+                throw TextException("transients entry missing ENTRY_REQUIRES_COLLAPSING", Here());
+            }
+
+            if (!transients->hasWriter(entry)) {
+                // prevent others from falling into the same trap
+                throw TextException("unattached transients entry missing writer", Here());
+            }
+        }
     }
 }