[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.