[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