Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Amos Jeffries]

On 9/09/2016 11:01 p.m., Eduard Bagdasaryan wrote:
> Made a couple of fixes:
> 
> * avoid needless updates when a [revalidation] reply lacks Last-Modified
> 
> * compilation errors while running test-builds.sh
> 
> Also attached a port to v3.5.

Applied to 4.x as rev.14845.

Amos


___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

Made a couple of fixes:

* avoid needless updates when a [revalidation] reply lacks Last-Modified

* compilation errors while running test-builds.sh

Also attached a port to v3.5.


Eduard.
Bug 4471: revalidation doesn't work when expired cached object lacks Last-Modified.

The bug was caused by the fact that Squid used only Last-Modified header
value for evaluating entry's last modification time while making an
internal revalidation request. So, without Last-Modified it was not
possible to correctly fill If-Modified-Since header value. As a result,
the revalidation request was not initiated when it should be.

To fix this problem, we should generate If-Modified-Since based on other
data, showing how "old" the cached response is, such as Date and Age
header values. Both Date and Age header values are utilized while
calculating entry's timestamp. Therefore, we should use the timestamp if
Last-Modified is unavailable.

TODO: HttpRequest::imslen support looks broken/deprecated. Using this
field inside StoreEntry::modifiedSince() leads to several useless checks
and probably may cause other [hidden] problems.

=== modified file 'src/MemStore.cc'
--- src/MemStore.cc	2016-05-01 21:37:52 +
+++ src/MemStore.cc	2016-09-08 14:11:18 +
@@ -429,61 +429,61 @@
 // already disconnected from the cache, no need to update
 if (index < 0)
 return true;
 
 if (!map)
 return false;
 
 const Ipc::StoreMapAnchor  = map->readableEntry(index);
 return updateCollapsedWith(collapsed, index, anchor);
 }
 
 /// updates collapsed entry after its anchor has been located
 bool
 MemStore::updateCollapsedWith(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 collapsed.swap_file_sz = anchor.basics.swap_file_sz;
 const bool copied = copyFromShm(collapsed, index, anchor);
 return copied;
 }
 
 /// anchors StoreEntry to an already locked map entry
 void
 MemStore::anchorEntry(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 const Ipc::StoreMapAnchor::Basics  = anchor.basics;
 
 e.swap_file_sz = basics.swap_file_sz;
 e.lastref = basics.lastref;
 e.timestamp = basics.timestamp;
 e.expires = basics.expires;
-e.lastmod = basics.lastmod;
+e.lastModified(basics.lastmod);
 e.refcount = basics.refcount;
 e.flags = basics.flags;
 
 assert(e.mem_obj);
 if (anchor.complete()) {
 e.store_status = STORE_OK;
 e.mem_obj->object_sz = e.swap_file_sz;
 e.setMemStatus(IN_MEMORY);
 } else {
 e.store_status = STORE_PENDING;
 assert(e.mem_obj->object_sz < 0);
 e.setMemStatus(NOT_IN_MEMORY);
 }
 assert(e.swap_status == SWAPOUT_NONE); // set in StoreEntry constructor
 e.ping_status = PING_NONE;
 
 EBIT_CLR(e.flags, RELEASE_REQUEST);
 EBIT_CLR(e.flags, KEY_PRIVATE);
 EBIT_SET(e.flags, ENTRY_VALIDATED);
 
 MemObject::MemCache  = e.mem_obj->memCache;
 mc.index = index;
 mc.io = MemObject::ioReading;
 }
 
 /// copies the entire entry from shared to local memory
 bool
 MemStore::copyFromShm(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 debugs(20, 7, "mem-loading entry " << index << " from " << anchor.start);

=== modified file 'src/Store.h'
--- src/Store.h	2016-07-23 13:36:56 +
+++ src/Store.h	2016-09-08 22:28:50 +
@@ -104,78 +104,89 @@
 const char *url() const;
 /// Satisfies cachability requirements shared among disk and RAM caches.
 /// Encapsulates common checks of mayStartSwapOut() and memoryCachable().
 /// TODO: Rename and make private so only those two methods can call this.
 bool checkCachable();
 int checkNegativeHit() const;
 int locked() const;
 int validToSend() const;
 bool memoryCachable(); ///< checkCachable() and can be cached in memory
 
 /// if needed, initialize mem_obj member w/o URI-related information
 MemObject *makeMemObject();
 
 /// initialize mem_obj member (if needed) and supply URI-related info
 void createMemObject(const char *storeId, const char *logUri, const HttpRequestMethod );
 
 void dump(int debug_lvl) const;
 void hashDelete();
 void hashInsert(const cache_key *);
 void registerAbort(STABH * cb, void *);
 void reset();
 void setMemStatus(mem_status_t);
 bool timestampsSet();
 void unregisterAbort();
 void destroyMemObject();
 int checkTooSmall();
 
 void delayAwareRead(const Comm::ConnectionPointer , char *buf, int len, AsyncCall::Pointer callback);
 
 void setNoDelay (bool const);
-bool modifiedSince(HttpRequest * request) const;
+void lastModified(const time_t when) { lastModified_ = when; }
+/// \returns entry's 'effective' modification time
+time_t lastModified() const {
+// may still return -1 if timestamp is not set
+return lastModified_ < 0 ? timestamp : lastModified_;
+}
+/// \returns a formatted string with entry's timestamps
+const 

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/30/2016 04:35 AM, Eduard Bagdasaryan wrote:
> 2016-08-28 1:12 GMT+03:00 Alex Rousskov : 
>> Not all HTCP clients are Squids, but how does Squid code treat such an
>> HTCP TST response?

> It seems that Squid does not care whether HTCP response contains LMT or
> not. Moreover, it looks that all TST response headers are only parsed
> inside htcpHandleTstResponse() without using them further for making
> any decisions

Wow. That finding combined with Amos assertion that
effectiveModificationTime() is always acceptable as the lastmod
value strongly suggests that there is no need to distinguish the two.
How about this:


   void lastModified(const time_t when) { lastModified_ = when; }
   time_t lastModified() const
   {
   // may still return -1 if timestamp is not set
   return lastModified_ < 0 ? timestamp : lastModified_;
   }

and completely removing effectiveModificationTime()?


I also think that we should rename/hide lastmod because it reduces the
chances that somebody will access the [negative] data member directly:


> /* START OF ON-DISK STORE_META_STD TLV field */
> time_t timestamp;
> time_t lastref;
> time_t expires;
> private:
> time_t lastModified_; ///< received Last-Modified value or -1; use 
> lastModified()
> public:
> uint64_t swap_file_sz;
> uint16_t refcount;
> uint16_t flags;
> /* END OF ON-DISK STORE_META_STD */


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

2016-08-28 1:12 GMT+03:00 Alex Rousskov :

> I worry about the message recipient comparing received
> effective LMT with the actual (absent!) LMT of the object they have in
> the cache and then deciding that the resource has changed (and their
> cached copy is now stale) because the resource now has LMT and it did
> not have it before.
>
> Not all HTCP clients are Squids, but how does Squid code treat such an
> HTCP TST response? In other words, will Squid itself make a different
> decision if it receives an effective LMT header value instead of no LMT
> header field at all? Does your own patch affect the receiving Squid
> behavior?

It seems that Squid does not care whether HTCP response contains LMT or
not. Moreover, it looks that all TST response headers are only parsed
inside htcpHandleTstResponse() without using them further for making
any decisions (probably they just intended to detect parse errors?).
Thus I see no problems for Squid as HTCP client when receiving effective
LMT in HTCP responses.


Eduard.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/27/2016 08:33 AM, Amos Jeffries wrote:

> If the response Squid would emit to the client proxy would contain a
> synthesized Last-Modified header - then the same synthetic value should
> be sent in HTCP.

I agree with that decision logic.


> I think Squid should be emitting a synthetic L-M header. So yes to
> effective modification time being sent (in both HTCP and HTTP responses).

I am worried that emitting effective/synthetic LMT would confuse some
clients that are currently working OK (possibly, but not necessarily,
including some Squids). After all, "unknown LMT" and "LMT of at most X"
are not exactly the same things, even if the protocol treats them the
same in many specific cases.

However, if Amos is sure that always emitting effective LMT [in cache
hits?] is a good idea, then I have no grounds for an objection.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/27/2016 05:22 AM, Eduard Bagdasaryan wrote:
> 2016-08-25 18:52 GMT+03:00 Alex Rousskov
> :
> 
>> 3. Sending an HTCP message to another service.
>>
>> > -hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod);
>> > +if (e && e->lastModified() > -1)
>> > +hdr.putTime(Http::HdrType::LAST_MODIFIED,
> e->lastModified());

>> Is this a conditional/revalidation request? If yes, then should we use
>> an effective modification time instead, like we do in use case #1?

> The code snippet is taken from htcpTstReply(), serving replies for TST
> requests (TST - test for the presence in the cache). If the entry is cached,
> this method fills HTCP reply packet with some of cached entry's headers (Age,
> Expires, Last-Modified etc.). According to the documentation, these headers 
> are
> added in order to give client an ability to calculate object's freshness 
> itself:
> HTCP response does not provide such information explicitly. On the other 
> hand, I
> have not found any strict requirements of what HTTP headers HTCP reply should
> contain.
> 
> Would the "effective" Last-Modified information help HTCP client to
> calculate object's freshness? It looks that some more information in this 
> case is
> better that lack of it.

Good question. I worry about the message recipient comparing received
effective LMT with the actual (absent!) LMT of the object they have in
the cache and then deciding that the resource has changed (and their
cached copy is now stale) because the resource now has LMT and it did
not have it before.

Not all HTCP clients are Squids, but how does Squid code treat such an
HTCP TST response? In other words, will Squid itself make a different
decision if it receives an effective LMT header value instead of no LMT
header field at all? Does your own patch affect the receiving Squid
behavior?


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Amos Jeffries]

On 27/08/2016 11:22 p.m., Eduard Bagdasaryan wrote:
> 2016-08-25 18:52 GMT+03:00 Alex Rousskov:
> 
>> 3. Sending an HTCP message to another service.
>>
>> > -hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod);
>> > +if (e && e->lastModified() > -1)
>> > +hdr.putTime(Http::HdrType::LAST_MODIFIED,
> e->lastModified());
>>
>> Is this a conditional/revalidation request? If yes, then should we use
>> an effective modification time instead, like we do in use case #1?
> 
> I have not found what is the "conditional/revalidation" request from
> HTCP point
> of view. The code snippet is taken from htcpTstReply(), serving replies
> for TST
> requests (TST - test for the presence in the cache). If the entry is
> cached,
> this method fills HTCP reply packet with some of cached entry's headers
> (Age,
> Expires, Last-Modified etc.). According to the documentation, these
> headers are
> added in order to give client an ability to calculate object's freshness
> itself:
> HTCP response does not provide such information explicitly. On the other
> hand, I
> have not found any strict requirements of what HTTP headers HTCP reply
> should
> contain.
> 
> Would the "effective" Last-Modified information help HTCP client to
> calculate
> object's freshness? It looks that some more information in this case is
> better
> that lack of it.

If the response Squid would emit to the client proxy would contain a
synthesized Last-Modified header - then the same synthetic value should
be sent in HTCP.

I think Squid should be emitting a synthetic L-M header. So yes to
effective modification time being sent (in both HTCP and HTTP responses).

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

2016-08-25 18:52 GMT+03:00 Alex Rousskov :

> 3. Sending an HTCP message to another service.
>
> > -hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod);
> > +if (e && e->lastModified() > -1)
> > +hdr.putTime(Http::HdrType::LAST_MODIFIED, 
e->lastModified());

>
> Is this a conditional/revalidation request? If yes, then should we use
> an effective modification time instead, like we do in use case #1?

I have not found what is the "conditional/revalidation" request from 
HTCP point
of view. The code snippet is taken from htcpTstReply(), serving replies 
for TST

requests (TST - test for the presence in the cache). If the entry is cached,
this method fills HTCP reply packet with some of cached entry's headers 
(Age,
Expires, Last-Modified etc.). According to the documentation, these 
headers are
added in order to give client an ability to calculate object's freshness 
itself:
HTCP response does not provide such information explicitly. On the other 
hand, I
have not found any strict requirements of what HTTP headers HTCP reply 
should

contain.

Would the "effective" Last-Modified information help HTCP client to 
calculate
object's freshness? It looks that some more information in this case is 
better

that lack of it.


Eduard.
___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Amos Jeffries]

On 26/08/2016 3:52 a.m., Alex Rousskov wrote:
> On 08/25/2016 04:04 AM, Eduard Bagdasaryan wrote:
> 
>> Therefore, we could use the timestamp if Last-Modified is unavailable.
> 
> I do not understand why the patch hides the lastmod field behind a basic
> getter. If we assert that a timestamp-based last modification value
> should be used in many cases, then we need to force the calling code to
> make an important choice between a raw lastmod and a timestamp-based
> last modification value.
> 
> The patch does not force the caller to make that choice because
> developers will naturally call lastModified() getter instead of
> effectiveModificationTime(). If calling lastModified() produces more
> problems like the one you are trying to fix (with reviewers not being
> able to guess that the wrong method was called), then renaming lastmod_
> accomplished nothing. Similarly, if calling lastModified() produces no
> new problems (because the lastmod usage case you were fixing happened to
> be exceptional rather than the norm), then renaming lastmod_ also
> accomplished nothing.
> 
> The patch does use effectiveModificationTime() in several places already
> so I suspect its usage is not that exceptional -- many, possibly even
> most contexts should use effectiveModificationTime(). If you agree, then
> we should make using lastmod_ getter more difficult -- the developer and
> the reviewer should be forced to pay attention to such use because it is
> likely to indicate a bug in new code (i.e., the new code should have
> called effectiveModificationTime() instead).
> 
> Moreover, these raw lastmod abuse problems might be already surfacing in
> your own patch! I see several raw lastmod value uses there:
> 
> 
> 1. Misleading debugging:
> 
>> +const time_t lastmod = http->storeEntry()->effectiveModificationTime();
>> +http->request->lastmod = lastmod;
> ...
>> -debugs(88, 5, "clientReplyContext::processExpired : lastmod " << 
>> entry->lastmod );
>> +debugs(88, 5, "lastmod " << entry->lastModified());
> 
> In other words, Squid is going to use the effective modification time,
> but we are logging raw time to make triage harder.
> 
> 
> 2. Writing -1 to cache store metadata:
> 
>>  currentEntry(sd->addDiskRestore(key,
>>  tmpe.timestamp,
> ...
>> -tmpe.lastmod,
>> +tmpe.lastModified(),
> 
> and
> 
>>  StoreSwapLogData s;
>>  s.timestamp = e.timestamp;
> ...
>> -s.lastmod = e.lastmod;
>> +s.lastmod = e.lastModified();
> 
> and
> 
>> -basics.lastmod = from.lastmod;
>> +basics.lastmod = from.lastModified();
> 
> Does a store need raw lastmod or effective modification time? I suspect
> it is the latter, but perhaps I am missing some valid use cases for the
> former. This needs a careful consideration.
> 
> If we are lucky, there is a relatively simple way to answer this
> question: If the stored lastmod value is only used to re-initialize some
> future StoreEntry::lastmod_ value _and_ all non-debugging uses of
> StoreEntry::lastmod_ are going to be via effectiveModificationTime(),
> then we can store effective modification time instead of -1!
> 
> 
> 3. Sending an HTCP message to another service.
> 
>> -hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod);
>> +if (e && e->lastModified() > -1)
>> +hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastModified());
> 
> Is this a conditional/revalidation request? If yes, then should we use
> an effective modification time instead, like we do in use case #1?


NP: From the protocol perspective Last-Modified can always be
synthesized from either Date or Date+Age, for *any* request. At worst it
will be conservatively *later* than the real (but absent) LM timestamp.

Thus effectiveModificationTime() is always acceptible as the lastmod
value. Even when its not taken from an actual L-M header.

AFAIK, the only actual need uses of lastModified() getter are when
checking for the headers explicit existence. Which should be doing so
via the HttpHeader object listing headers, not the Store metadata
(calculated) values.

Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/25/2016 04:04 AM, Eduard Bagdasaryan wrote:

> Therefore, we could use the timestamp if Last-Modified is unavailable.

I do not understand why the patch hides the lastmod field behind a basic
getter. If we assert that a timestamp-based last modification value
should be used in many cases, then we need to force the calling code to
make an important choice between a raw lastmod and a timestamp-based
last modification value.

The patch does not force the caller to make that choice because
developers will naturally call lastModified() getter instead of
effectiveModificationTime(). If calling lastModified() produces more
problems like the one you are trying to fix (with reviewers not being
able to guess that the wrong method was called), then renaming lastmod_
accomplished nothing. Similarly, if calling lastModified() produces no
new problems (because the lastmod usage case you were fixing happened to
be exceptional rather than the norm), then renaming lastmod_ also
accomplished nothing.

The patch does use effectiveModificationTime() in several places already
so I suspect its usage is not that exceptional -- many, possibly even
most contexts should use effectiveModificationTime(). If you agree, then
we should make using lastmod_ getter more difficult -- the developer and
the reviewer should be forced to pay attention to such use because it is
likely to indicate a bug in new code (i.e., the new code should have
called effectiveModificationTime() instead).

Moreover, these raw lastmod abuse problems might be already surfacing in
your own patch! I see several raw lastmod value uses there:


1. Misleading debugging:

> +const time_t lastmod = http->storeEntry()->effectiveModificationTime();
> +http->request->lastmod = lastmod;
...
> -debugs(88, 5, "clientReplyContext::processExpired : lastmod " << 
> entry->lastmod );
> +debugs(88, 5, "lastmod " << entry->lastModified());

In other words, Squid is going to use the effective modification time,
but we are logging raw time to make triage harder.


2. Writing -1 to cache store metadata:

>  currentEntry(sd->addDiskRestore(key,
>  tmpe.timestamp,
...
> -tmpe.lastmod,
> +tmpe.lastModified(),

and

>  StoreSwapLogData s;
>  s.timestamp = e.timestamp;
...
> -s.lastmod = e.lastmod;
> +s.lastmod = e.lastModified();

and

> -basics.lastmod = from.lastmod;
> +basics.lastmod = from.lastModified();

Does a store need raw lastmod or effective modification time? I suspect
it is the latter, but perhaps I am missing some valid use cases for the
former. This needs a careful consideration.

If we are lucky, there is a relatively simple way to answer this
question: If the stored lastmod value is only used to re-initialize some
future StoreEntry::lastmod_ value _and_ all non-debugging uses of
StoreEntry::lastmod_ are going to be via effectiveModificationTime(),
then we can store effective modification time instead of -1!


3. Sending an HTCP message to another service.

> -hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastmod);
> +if (e && e->lastModified() > -1)
> +hdr.putTime(Http::HdrType::LAST_MODIFIED, e->lastModified());

Is this a conditional/revalidation request? If yes, then should we use
an effective modification time instead, like we do in use case #1?


4. StoreEntry state reporting.

>  describeTimestamps(const StoreEntry * entry)
...
>   (int) entry->timestamp,
...
> - (int) entry->lastmod,
> + (int) entry->lastModified(),

The debugging should reflect the true value so this is fine. However, we
can make this function a StoreEntry method to avoid the need for getter
_if_ this is the only place where the getter is needed.


If I did not miss any use cases, then #3 appears to be critical here. If
we should be using effective modification time in #3, then there appears
to be no need to store -1 in StoreEntry and your patch can be greatly
simplified. If #3 does need a raw value from StoreEntry, then we will
need a better way to force developers to think about the right method to
use.

Please double check what is going on in #3, and we will go from there.


Thank you,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

2016-08-21 15:58 GMT+03:00 Amos Jeffries :

> please dont move the StoreEntry 'lastmod' member variable which exists
> between the "ON-DISK STORE_META_STD TLV field" marker comments.

> A delta should be the actual difference value -N < 0 < +N.

> please take the opportunity to cleanup any touched debugs() messages.

> please add a note "XXX BUG 1890 objects without Date do not get one
> added".

Addressed these suggestions and refreshed the patch.


Eduard.
Bug 4471: revalidation doesn't work when expired cached object lacks Last-Modified.

The bug was caused by the fact that Squid used only Last-Modified header
value for evaluating entry's last modification time while making an
internal revalidation request. So, without Last-Modified it was not
possible to correctly fill If-Modified-Since header value. As a result,
the revalidation request was not initiated when it should be.

To fix this problem, we should generate If-Modified-Since based on other
data, showing how "old" the cached response is, such as Date and Age
header values. Both Date and Age header values are utilized while
calculating entry's timestamp. Therefore, we could use the timestamp if
Last-Modified is unavailable.

TODO: HttpRequest::imslen support looks broken/deprecated. Using this
field inside StoreEntry::modifiedSince() leads to several useless checks
and probably may cause other [hidden] problems.

=== modified file 'src/MemStore.cc'
--- src/MemStore.cc	2016-05-01 21:37:52 +
+++ src/MemStore.cc	2016-08-25 06:30:42 +
@@ -429,61 +429,61 @@
 // already disconnected from the cache, no need to update
 if (index < 0)
 return true;
 
 if (!map)
 return false;
 
 const Ipc::StoreMapAnchor  = map->readableEntry(index);
 return updateCollapsedWith(collapsed, index, anchor);
 }
 
 /// updates collapsed entry after its anchor has been located
 bool
 MemStore::updateCollapsedWith(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 collapsed.swap_file_sz = anchor.basics.swap_file_sz;
 const bool copied = copyFromShm(collapsed, index, anchor);
 return copied;
 }
 
 /// anchors StoreEntry to an already locked map entry
 void
 MemStore::anchorEntry(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 const Ipc::StoreMapAnchor::Basics  = anchor.basics;
 
 e.swap_file_sz = basics.swap_file_sz;
 e.lastref = basics.lastref;
 e.timestamp = basics.timestamp;
 e.expires = basics.expires;
-e.lastmod = basics.lastmod;
+e.lastModified(basics.lastmod);
 e.refcount = basics.refcount;
 e.flags = basics.flags;
 
 assert(e.mem_obj);
 if (anchor.complete()) {
 e.store_status = STORE_OK;
 e.mem_obj->object_sz = e.swap_file_sz;
 e.setMemStatus(IN_MEMORY);
 } else {
 e.store_status = STORE_PENDING;
 assert(e.mem_obj->object_sz < 0);
 e.setMemStatus(NOT_IN_MEMORY);
 }
 assert(e.swap_status == SWAPOUT_NONE); // set in StoreEntry constructor
 e.ping_status = PING_NONE;
 
 EBIT_CLR(e.flags, RELEASE_REQUEST);
 EBIT_CLR(e.flags, KEY_PRIVATE);
 EBIT_SET(e.flags, ENTRY_VALIDATED);
 
 MemObject::MemCache  = e.mem_obj->memCache;
 mc.index = index;
 mc.io = MemObject::ioReading;
 }
 
 /// copies the entire entry from shared to local memory
 bool
 MemStore::copyFromShm(StoreEntry , const sfileno index, const Ipc::StoreMapAnchor )
 {
 debugs(20, 7, "mem-loading entry " << index << " from " << anchor.start);

=== modified file 'src/Store.h'
--- src/Store.h	2016-07-23 13:36:56 +
+++ src/Store.h	2016-08-25 09:16:52 +
@@ -104,78 +104,87 @@
 const char *url() const;
 /// Satisfies cachability requirements shared among disk and RAM caches.
 /// Encapsulates common checks of mayStartSwapOut() and memoryCachable().
 /// TODO: Rename and make private so only those two methods can call this.
 bool checkCachable();
 int checkNegativeHit() const;
 int locked() const;
 int validToSend() const;
 bool memoryCachable(); ///< checkCachable() and can be cached in memory
 
 /// if needed, initialize mem_obj member w/o URI-related information
 MemObject *makeMemObject();
 
 /// initialize mem_obj member (if needed) and supply URI-related info
 void createMemObject(const char *storeId, const char *logUri, const HttpRequestMethod );
 
 void dump(int debug_lvl) const;
 void hashDelete();
 void hashInsert(const cache_key *);
 void registerAbort(STABH * cb, void *);
 void reset();
 void setMemStatus(mem_status_t);
 bool timestampsSet();
 void unregisterAbort();
 void destroyMemObject();
 int checkTooSmall();
 
 void delayAwareRead(const Comm::ConnectionPointer , char *buf, int len, AsyncCall::Pointer callback);
 
 void setNoDelay (bool const);
-bool modifiedSince(HttpRequest * request) const;
+time_t effectiveModificationTime() const {
+return 

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Alex Rousskov]

On 08/23/2016 09:17 AM, Amos Jeffries wrote:
> On 24/08/2016 12:07 a.m., Eduard Bagdasaryan wrote:
>> 2016-08-21 15:58 GMT+03:00 Amos Jeffries :
>>> To change anything between those markers we have to do a full cache
>>> versioning and up/down-grade compatibility dance.

>> Could you please clarify what versioning problems you are talking
>> about?

> I was referring to that block of members being read and written
> directly to cache files. Either swap.state or the object metadata section.
> I dont think they are referenced individully, but as a raw-pointer to
> the first members address cast to some other identically laid out
> StoreMeta* type (yucky).


Yes, this happens in at least two places:

> bzr grep timestamp src | fgrep \&
...
> src/store_rebuild.cc:memcpy(>timestamp, x.value, 
> STORE_HDR_METASIZE);
> src/store_swapmeta.cc:t = 
> StoreMeta::Factory(STORE_META_STD_LFS,STORE_HDR_METASIZE,>timestamp)


> The point of those comments AFAIK is to make it obvious that kind of
> this is/was being done with them and prevent us doing what you did.

Agreed. Since improving this code is way outside this simple fix scope,
I recommend returning the renamed lastmod data member into that "keep
them together" section, with a comment:

time_t lastmod_; ///< received Last-Modified value or -1; treat as private

Please note that "received Last-Modified value or -1" is just my guess
what lastmod_ is -- use the right description if my guess is wrong.

Renaming lastmod (and temporary making it private) already forced you to
check and, if needed, adjust most of the old code using that data
member. Keeping lastmod_ public is not ideal, but it does not make
things worse and allows you to stay focused. Fixing related problems is
only a good idea when those fixes are simple and do not stray you off
course too much.


HTH,

Alex.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Eduard Bagdasaryan]

2016-08-21 15:58 GMT+03:00 Amos Jeffries :

> To change anything between those markers we have to do a full cache
> versioning and up/down-grade compatibility dance.

Could you please clarify what versioning problems you are talking about?  It
seems that StoreEntry's STORE_META_STD fields are not used directly while
storing/restoring metainformation. I found only places where field-by-field
assignment is performed (e.g., InitStoreEntry::operator()), so making
StoreEntry::lastmod 'private' should not do any harm there.  So probably we
could mark with  "START OF ON-DISK STORE_META_STD TLV field" (or 
similar) the

StoreEntry::lastmod, which is now private.

> lastModifiedDelta() returning -1 when the actual delta is any -N value
> is wrong conceptually. A delta should be the actual difference value 
-N < 0 < +N.


Since we need and use only the positive outcome of this method inside
refreshStaleness(), probably rename it to ageWhenCached() (or similar)?
Returning '-1' would mean that it is impossible to calculate the age 
('lastmod'

is absent or greater than timestamp).


Eduard.

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev

Re: [squid-dev] [PATCH] Revalidate without Last-Modified

[Amos Jeffries]

On 21/08/2016 9:08 a.m., Eduard Bagdasaryan wrote:
> Hello,
> 
> This patch fixes bug 4471.
> 
> The bug was caused by the fact that Squid used only Last-Modified header
> value for evaluating entry's last modification time while making an
> internal revalidation request. So, without Last-Modified it was not
> possible to correctly fill If-Modified-Since header value. As a result,
> the revalidation request was not initiated when it should be.
> 
> To fix this problem, we should generate If-Modified-Since based on other
> data, showing how "old" the cached response is, such as Date and Age
> header values.  Both Date and Age header values are utilized while
> calculating entry's timestamp.  Therefore, we could use the timestamp if
> Last-Modified is unavailable.
> 
> XXX: HttpRequest::imslen support looks broken/deprecated. Using this
> field inside StoreEntry::modifiedSince() leads to several useless checks
> and probably may cause other [hidden] problems.
> 

Thanks for this.

In src/Store.h:

* please dont move the StoreEntry 'lastmod' member variable which exists
between the "ON-DISK STORE_META_STD TLV field" marker comments.

 - To change anything between those markers we have to do a full cache
versioning and up/down-grade compatibility dance. I think its best to
keep all that trouble separate from this patch.
 - for now just document it with a comment saying how it should to be
set instead of by assignment.


in src/store.cc:

* lastModifiedDelta() returning -1 when the actual delta is any -N value
is wrong conceptually.
 - A delta should be the actual difference value -N < 0 < +N.
 - I've not had time right now to check how fixing that affects the
caller logic though. That check will need doing before any commit so we
can either produce proper delta's or comment why they get truncated to
-1 in half the cases.


in src/client_side_reply.cc:

* please take the opportunity to cleanup any touched debugs() messages.
Like this one:
 debugs(88, 5, "clientReplyContext::processExpired : lastmod " <<
entry->lastModified() );

should be:
 debugs(88, 5, "lastmod " << entry->lastModified());

- note function name and whitespace before ending bracket are gone.

* for the hunk @@ -587 where it says "cannot revalidate entries without
timestamp or Last-Modified header"
 - please add a note "XXX BUG 1890 objects without Date do not get one
added". I think when that compliance bug is fixed an effective-LM
will/should always be found.

Cheers
Amos

___
squid-dev mailing list
squid-dev@lists.squid-cache.org
http://lists.squid-cache.org/listinfo/squid-dev