Unable to serve from cache if cache entry was expired or invalidated

[vankoven]

The issue is here: https://github.com/tempesta-tech/tempesta/blob/master/tempesta_fw/cache.c#L852

__cache_add_node() must revalidate or remove already existing entry to the key given instead of unconditional addition of a new cache entry. The new cache entry will have the same key as the expired one, so the new entry will be added into one collision chain with expired entry. In most cases it will be added to the end of the chain.

This make impossible to get not expired cache entry when serving new requests. And response to every request of that resource will be added into cache. That leads to memory leakage.

Steps to reproduce:

1. Configure backend to have small expire timeout and disable caching on backend. E.g. for nginx:

expires 15; # 15 seconds

2. Send GET to TempestaFW. It will be forwarded to origin server, response will populate the cache
3. Send GET once again, it will be served from cache
4. Wait 15-20 secconds, cache entry will expire
5. Send GET, It will be forwarded to origin server, response will populate the cache
6. Send GET once again, It will be forwarded to origin server, response will populate the cache. That is the issue, the request must be served from cache

Note: there is a few more tasks on cache revalidation: #515 #518

Depends on #515 (TDBv2, removal is required).

[krizhanovsky]

Actually we must keep stale responses in the cache, but servicing stale responses is the subject of #522 .

The problem described in this issues seems can be easily fixed with checking whether the found cache entry ce is stale in the loop in tfw_cache_dbce_get(). If it is stale, then just store it in some temporary variable stale_ce and proceed to the next entry. If no fresh entry is found, then return the entry from stale_ce. Having that multiple stale entries can be stored, the latest one must be remembered by stale_ce. Thus, the tfw_cache_entry_is_live() predicate should be moved from cache_req_process_node() to tfw_cache_dbce_get().

Remembering stale_ce might be dangerous with cache removal (see #515), so please add a comment TODO #515: add records locking and/or reference counting.

[ghost]

I've added a PR #1532 that implements more or less the idea described in your comment.

I did some simple manual testing, and the automated "cache" tests also pass.

Completely removing the liveness check from cache_req_process_node would mean that (with the way everything works right now) if we have any cache record that is stale (for example, because it was just purged), then this record will always be served to a client and we never update from an upstream. At least, until #522

Actually I think that right now it isn't very useful to return stale entries, because they aren't used. But it will make more sense with 522.

Also can you please clarify where the comment about record locking/RC should go? It seems we already take a bucket lock when iterating, and anyway we'd probably have to review all uses of TDB when implementing deletion.

[krizhanovsky]

Yeah, I agree: let's not to return stale records from tfw_cache_dbce_get() for now, let's leave it for #522. Anyway now it seems that if tfw_cache_dbce_get() returns a stale entry, tfw_cache_entry_is_live() in cache_req_process_node() doesn't allow to proceed with it.

Also can you please clarify where the comment about record locking/RC should go? It seems we already take a bucket lock when iterating, and anyway we'd probably have to review all uses of TDB when implementing deletion.

Yeah, we don't need it. I just didn't touch the code for a while :)