Fix to correctly serve cached pages if we're caching on first visit.

[aidvu]

When times < 2 or seconds < 1 it would always recache the pages, even tho they should be served from cache. Needed an expired check.

Testing Instructions:

1. Set times = 1, or seconds = 0
2. Visit any page that's cacheable
3. Check HTML comment, you'll see it's been set
4. Refresh page

Before:

5. The page isn't served from cache. It's set again.

After:

5. Served from cache

[david-binda]

Good find @aidvu , I was able to reproduce the issue.

However, I'm not sure the proposed fix the best one. What feels like an issue is the genlock how we are checking it.

Even if the following conditional passes:

https://github.com/Automattic/batcache/blob/1c5cd87c3da8c0224b13f51f9f3362eccb1c3811/advanced-cache.php#L462 and $batcache->do is set to true, the request should, IMHO, be covered by batcache, in case all the conditions in following link pass:

https://github.com/Automattic/batcache/blob/1c5cd87c3da8c0224b13f51f9f3362eccb1c3811/advanced-cache.php#L486,L492

The conditions, speaking in human language, are:

1. we have cache
2. we have not obtained cache regeneration lock
3. Batcached page that hasn't expired OR Regenerating it in another request and can use stale cache

What I believe is the bug here, is the translation of the number 2 to code ( we have not obtained cache generation lock ). My understanding is that the cache generation lock is being set during the time we are rebuilding the cache, so all the requests coming to the server at time the cache is being generated, only spawn a single cache regeneration process, while the others are using stale cache till the time regeneration finishes.

Here's the problem with that conditional check:

https://github.com/Automattic/batcache/blob/1c5cd87c3da8c0224b13f51f9f3362eccb1c3811/advanced-cache.php#L484

wp_cache_add returns true if the memcache add operation was successful (as in: there is no other process locking the generation).

and we check the lock against false value in

https://github.com/Automattic/batcache/blob/1c5cd87c3da8c0224b13f51f9f3362eccb1c3811/advanced-cache.php#L487

However, the $batcache->genlock is only going to be false in case the memcache add operation failed, as there is another process currently regenerating the cache.

That said, in case our process was able to set the genlock, it means that we can use the cache. But also, that the genlock is set for other process for 10 seconds. From my point of view, we should take advantage of wp_cache_get for checking whether there is a genlock or not, instead of using the add operation, and only add the genlock, in case batcache is really being regenerated.

[david-binda]

Turns out that my understanding of what the $batcache->do means is incorrect. It's meaning was somewhat shifted in #46 and now can be understood as "regenerate".

Which mean, that my above comment does not make much sense :) Thanks @aidvu for pointing that out! I'm going to re-review the PR with a different optics now.