On 07/17/2016 12:59 PM, Alex Rousskov wrote: > On 07/17/2016 05:01 AM, Amos Jeffries wrote: >> I've just been looking at the Store::Controller::find() implementation >> and it struck me that if the transients lookup has an error the object >> will fail to HIT on any existing cache entries. > > If the transients table tells us that the transient object is in a "bad" > state, then trying to load that same object from a store will fail at > best or result in a stale/truncated/stuck response at worse. > > >> Alex; am I missing something undocumented here ? > > You might be missing something documented: The Transients definition or > purpose. Transients is not one of the many "if not here than possibly > there" cache stores. Transients is dedicated to cache entries in > transient state. If a given entry can be correctly loaded from a regular > cache store, then, by definition, that entry is not transient [any more] > and would not be in Transients. Consequently, if the entry is in > Transients, then it is impossible to load it correctly from a regular store. > > It is possible that a lock contention or a similar SMP race condition > inside Transients would result in a cache miss instead of reading from a > Transients-controlled entry, but, bugs notwithstanding, that should not > happen often.
And one more problem: The email subject implies that you are modifying handling of Transient hash collisions whereas your patch modifies handling of problematic Transient entries, not key collisions: > // Must search transients before caches because we must sync those > we find. > if (transients) { > if (StoreEntry *e = transients->get(key)) { > debugs(20, 3, "got shared in-transit entry: " << *e); > bool inSync = false; > const bool found = anchorCollapsed(*e, inSync); > if (!found || inSync) > return e; > assert(!e->locked()); // ensure release will > destroyStoreEntry() > e->release(); // do not let others into the same trap > - return NULL; > + // continue on to maybe find it in cache > } > } On hash collisions, transients->get() returns nil, bypassing your modifications: > StoreEntry * > Transients::get(const cache_key *key) > { ... > const Ipc::StoreMapAnchor *anchor = map->openForReading(key, index); > if (!anchor) > return NULL; > HTH, Alex. _______________________________________________ squid-dev mailing list squid-dev@lists.squid-cache.org http://lists.squid-cache.org/listinfo/squid-dev