Do not populate StoreEntry basics from Transients (#1343)

    client_side_reply.cc:1078: "http->storeEntry()->objectLen() >= 0"
    inside clientReplyContext::storeOKTransferDone()

Since commit 24c9378, Transients are not supposed to maintain "basic"
cache entry information (i.e. StoreMapAnchor::Basics a.k.a. StoreEntry
STORE_META_STD fields). Using often-at-their-defaults Transient basics
may unexpectedly erase valuable StoreEntry information obtained from one
of the caches (e.g., swap_file_sz) and lead to assertions.

Consequently, do not populate unused Transients entry basics either.

Eventually, we may find a good way to parameterize StoreMapAnchor to
avoid keeping those unused fields in Transients StoreMap.

diff --git a/src/MemObject.h b/src/MemObject.h
index adc786e8208cb1845dc13635d29a86cb1de510a9..0a7be72046574958a4f222b8fd2d35067c46ee25 100644 (file)
--- a/src/MemObject.h
+++ b/src/MemObject.h
@@ -169,6 +169,20 @@ public:
     class XitTable
     {
     public:
+        /// associate our StoreEntry with a Transients entry at the given index
+        void open(const int32_t anIndex, const Io anIo)
+        {
+            index = anIndex;
+            io = anIo;
+        }
+
+        /// stop associating our StoreEntry with a Transients entry
+        void close()
+        {
+            index = -1;
+            io = Store::ioDone;
+        }
+
         int32_t index = -1; ///< entry position inside the in-transit table
         Io io = ioUndecided; ///< current I/O state
     };

diff --git a/src/Transients.cc b/src/Transients.cc
index 16575606a3914f370c7af4b37ff24c9ad7142146..fbcf832c1d3045bd129a10cb68d455e177bbdc58 100644 (file)
--- a/src/Transients.cc
+++ b/src/Transients.cc
@@ -166,7 +166,7 @@ Transients::get(const cache_key *key)
 
     StoreEntry *e = new StoreEntry();
     e->createMemObject();
-    anchorEntry(*e, index, *anchor);
+    e->mem_obj->xitTable.open(index, Store::ioReading);
 
     // keep read lock to receive updates from others
     return e;
@@ -234,11 +234,9 @@ Transients::addWriterEntry(StoreEntry &e, const cache_key *key)
 
     // set ASAP in hope to unlock the slot if something throws
     // and to provide index to such methods as hasWriter()
-    auto &xitTable = e.mem_obj->xitTable;
-    xitTable.index = index;
-    xitTable.io = Store::ioWriting;
+    e.mem_obj->xitTable.open(index, Store::ioWriting);
 
-    anchor->set(e, key);
+    anchor->setKey(key);
     // allow reading and receive remote DELETE events, but do not switch to
     // the reading lock because transientReaders() callers want true readers
     map->startAppending(index);
@@ -250,27 +248,14 @@ void
 Transients::addReaderEntry(StoreEntry &e, const cache_key *key)
 {
     sfileno index = 0;
-    const auto anchor = map->openOrCreateForReading(key, index, e);
+    const auto anchor = map->openOrCreateForReading(key, index);
     if (!anchor)
         throw TextException("reader collision", Here());
 
-    anchorEntry(e, index, *anchor);
+    e.mem_obj->xitTable.open(index, Store::ioReading);
     // keep the entry locked (for reading) to receive remote DELETE events
 }
 
-/// fills (recently created) StoreEntry with information currently in Transients
-void
-Transients::anchorEntry(StoreEntry &e, const sfileno index, const Ipc::StoreMapAnchor &anchor)
-{
-    // set ASAP in hope to unlock the slot if something throws
-    // and to provide index to such methods as hasWriter()
-    auto &xitTable = e.mem_obj->xitTable;
-    xitTable.index = index;
-    xitTable.io = Store::ioReading;
-
-    anchor.exportInto(e);
-}
-
 bool
 Transients::hasWriter(const StoreEntry &e)
 {
@@ -362,8 +347,7 @@ Transients::disconnect(StoreEntry &entry)
             map->closeForReadingAndFreeIdle(xitTable.index);
         }
         locals->at(xitTable.index) = nullptr;
-        xitTable.index = -1;
-        xitTable.io = Store::ioDone;
+        xitTable.close();
     }
 }
 

diff --git a/src/Transients.h b/src/Transients.h
index e05fdb73bdf23c423bfff76980cea74722f8af7c..9ba325e7652b4524c25734cf4c8928e18df2aaee 100644 (file)
--- a/src/Transients.h
+++ b/src/Transients.h
@@ -94,7 +94,6 @@ protected:
     void addEntry(StoreEntry*, const cache_key *, const Store::IoStatus);
     void addWriterEntry(StoreEntry &, const cache_key *);
     void addReaderEntry(StoreEntry &, const cache_key *);
-    void anchorEntry(StoreEntry &, const sfileno, const Ipc::StoreMapAnchor &);
 
     // Ipc::StoreMapCleaner API
     void noteFreeMapSlice(const Ipc::StoreMapSliceId sliceId) override;

diff --git a/src/ipc/StoreMap.cc b/src/ipc/StoreMap.cc
index 89d80c8a01d59816bafa309cfb75677c6a3097f5..50bcbb9960cf5402c89a1159d73588e2d3e1f86b 100644 (file)
--- a/src/ipc/StoreMap.cc
+++ b/src/ipc/StoreMap.cc
@@ -100,7 +100,7 @@ Ipc::StoreMap::forgetWritingEntry(sfileno fileno)
 }
 
 const Ipc::StoreMap::Anchor *
-Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &fileno, const StoreEntry &entry)
+Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &fileno)
 {
     debugs(54, 5, "opening/creating entry with key " << storeKeyText(key)
            << " for reading " << path);
@@ -115,7 +115,7 @@ Ipc::StoreMap::openOrCreateForReading(const cache_key *const key, sfileno &filen
     // the competing openOrCreateForReading() workers race to create a new entry
     idx = fileNoByKey(key);
     if (auto anchor = openForWritingAt(idx)) {
-        anchor->set(entry, key);
+        anchor->setKey(key);
         anchor->lock.switchExclusiveToShared();
         // race ended
         assert(anchor->complete());

diff --git a/src/ipc/StoreMap.h b/src/ipc/StoreMap.h
index 255c713ec521fb342a7044aa73be06257ee542bd..616e3e5a2b4eca210541824b9c2604a41f3d10e6 100644 (file)
--- a/src/ipc/StoreMap.h
+++ b/src/ipc/StoreMap.h
@@ -314,7 +314,7 @@ public:
     void closeForReadingAndFreeIdle(const sfileno fileno);
 
     /// openForReading() but creates a new entry if there is no old one
-    const Anchor *openOrCreateForReading(const cache_key *, sfileno &, const StoreEntry &);
+    const Anchor *openOrCreateForReading(const cache_key *, sfileno &);
 
     /// writeable slice within an entry chain created by openForWriting()
     Slice &writeableSlice(const AnchorId anchorId, const SliceId sliceId);