refresh.cc - refreshIsCachable()

Hi all

Looking at a different function in refresh.cc - refreshIsCachable()

This function is only called in one place (in http.cc) and is used
there as part of an algorithm to determine whether a response from an
origin server should be cached.

int
refreshIsCachable(const StoreEntry * entry)
{
     /*
      * Don't look at the request to avoid no-cache and other nuisances.
      * the object should have a mem_obj so the URL will be found there.
      * minimum_expiry_time seconds delta (defaults to 60 seconds), to
      * avoid objects which expire almost immediately, and which can't
      * be refreshed.
      */
     int reason = refreshCheck(entry, NULL, Config.minimum_expiry_time);
     refreshCounts[rcStore].total++;
     refreshCounts[rcStore].status[reason]++;
        
     if (reason < 200)
         /* Does not need refresh. This is certainly cachable */
         return 1;

     if (entry->lastmod < 0)
         /* Last modified is needed to do a refresh */
         return 0;

     if (entry->mem_obj == NULL)
         /* no mem_obj? */
         return 1;

     if (entry->getReply() == NULL)
         /* no reply? */
         return 1;

     if (entry->getReply()->content_length == 0)
         /* No use refreshing (caching?) 0 byte objects */
         return 0;

     /* This seems to be refreshable. Cache it */
     return 1;
}

It looks like the call to refreshCheck() is only used to short-
circuit the function (if you can call it a short circuit, given the
size of the callout) and to update some statistics.

However, the return value can be calculated without calling it, and
the statistic it updates is never used. Would something like this be
cleaner/quicker and still correct?

int
refreshIsCachable(const StoreEntry * entry)
{
     if (entry->lastmod < 0)
         /* Last modified is needed to do a refresh */
         return 0;

     if (entry->getReply() && entry->getReply()->content_length == 0)
         /* No use refreshing (caching?) 0 byte objects */
         return 0;

     /* This seems to be cacheable */
     return 1;
}

The only potential problems I can see are if refreshCheck() does more
than just check (i.e. changes state in some important way I haven't
seen) or if it is somehow essential to report the "reason" statistics
in this function call. I can't see the reason stats being used
anywhere, but maybe I overlooked something. (And anyway... should
this function even be updating the refreshCounts[rcStore].total stat,
given the fact this just checks a couple of flags?)

Also, if the above is correct, then it looks like a wrong assumption
has been made in http.cc:

     case HTTP_OK:
     case HTTP_NON_AUTHORITATIVE_INFORMATION:
     case HTTP_MULTIPLE_CHOICES:
     case HTTP_MOVED_PERMANENTLY:
     case HTTP_GONE:
         /*
          * Don't cache objects that need to be refreshed on next
request,
          * unless we know how to refresh it.
          */
         if (!refreshIsCachable(entry))
             return 0;

Because the call to refreshIsCachable() doesn't say anything about
the response needing to be refreshed or not.

Is this right?

Cheers
D
Received on Mon Apr 24 2006 - 18:34:26 MDT