refresh.cc - refreshIsCachable()

[Doug Dixon]

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