Logic to abort cache clear not working as expected

There is a bug in the code. This patch should solve the problem.

It will be in the next release when it's status has been set to "Reviewed and tested by community". If you think this fix works for you, you should change the status of this issue. Once that is set, it is an indicator that this is ready to be commited. It is best practice that I don't set this issue to RTBC or fixed myself, since I was the one who created the patch. Once set to Reviewed and tested, I can commit this patch. It will be in the next dev release after 12 hours.

This is finally fixed! Sorry for the wait.

As a note for anyone coming across this, this was not rolled into Beta 1, but rather removed right before beta 1 was released in this commit

Then someone else came across this issue and it has been committed to dev

I'm getting this with an Acquia hosting account on a shared dev server. I've tailored the settings.php file so that the Varnish cache backend is only loaded on production and varnish_cache_clear is set to 0 on dev, but it still tries contacting the (non-existent) varnish server on 127.0.0.1:6082 when I run "drush cc all".

Trying out this patch to see if it will work - it changes the if() statement from:

    if (!module_exists('varnish') ||
      (variable_get('varnish_flush_cron', 0) && lock_may_be_available('cron')) && !variable_get('varnish_cache_clear', 1)) {
      return;
    }

to:

    if (!module_exists('varnish') ||
      !variable_get('varnish_cache_clear', 1) ||
      (variable_get('varnish_flush_cron', 0) && lock_may_be_available('cron'))) {
      return;
    }

This checks to see if the varnish_cache_clear variable is set to 0, i.e. cache_clear integration is disabled, which is IMHO what it should be doing.

Changing status to "needs review", as there is a new patch.

Patch in #11 fails for me for two reasons (as far as I can tell). First, lock_may_be_available('cron') is returning false and variable_get('varnish_flush_cron', 0) isn't actually doing a comparision, its just returning whatever varnish_flush_cron is set to.

I've rolled a new patch that fixes these issues, its possible I've misunderstood something about this process and if so I'd be very happy to be enlightened :)

Thanks,
Sam

I'd love to see this issue get resolved. I couldn't test it on SimplyTest.me, but the patch seemed to apply nicely which is a good start.

DUPLICATE

Updating the issue title to reflect the fact this not only affects what you'd expect to be a global setting (never clear the cache, even on cron) but also that the don't-clear-on-cron setting currently has no effect.

I re-rolled the patch in #13 to clarify the last two tests and make them consistent with the settings file. Interestingly, there's a simple 0/1 option for varnish_flush_cron, but the same 0/1 options are provided by constants in the form element for varnish_cache_clear. Just in case they ever change, I've used the constants here.

The test that's currently in the -dev branch to abort clearing the cache doesn't look right to me:

(!module_exists('varnish') || (variable_get('varnish_flush_cron', 0) && lock_may_be_available('cron')) && !variable_get('varnish_cache_clear', 1))

which I interpret to mean, don't proceed if:

• Varnish module is disabled, OR
• we do want to clear cache on cron and we can get the cron lock, AND we don't want to clear the cache.

This line was inserted with this commit from #1461792: Cache doesn't clear if "varnish_flush_cron" set to 1 but it didn't elicit much discussion or testing at the time.

Also marking as major since this results in Varnish not working as advertised/configured and could significantly limit your cache hit rate if you run cron frequently.

Apologies - the cron-related conditional should be checking if we want to clear caches on cron, AND that cron is currently running. So we look for the semaphore. The use of lock_may_be_available() doesn't seem to me like it would work; if cron is running it will have the lock, so it's unavailable to us.

EDIT: This is an interdiff, my mistake - see next comment for complete patch.

Here's the self-contained patch. Sorry to clog up the comment queue.

Updated: Apparently cron_semaphore is a relic of Drupal 6, even though D7 cleans it up after running system cron. Regardless, this still needs a fix and lock_may_be_available() is probably the best way to test for cron currently running.

The patch applies nicely. I'm trying to test it though and not sure the best approach.

@mgifford - you're not sure the right approach to test it, you mean?

Locally, I had a window open running a tail -f /var/log/syslog | grep varnish, which allowed me to view the CLI requests being run from varnish module.

In another window or browser, run drush/UI commands that would trigger varnish in various circumstances.

So using this patch with drush cc all I get:

Jan 13 23:08:46 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49601 127.0.0.1 6082 Rd auth LONGRANDOMSTRING
Jan 13 23:08:46 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49601 127.0.0.1 6082 Wr 200 -----------------------------#012Varnish Cache CLI 1.0#012-----------------------------#012Linux,3.2.0-56-generic,x86_64,-smalloc,-smalloc,-hcritbit#012#012Type 'help' for command list.#012Type 'quit' to close CLI session.
Jan 13 23:08:46 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49601 127.0.0.1 6082 Rd ban req.http.host ~ joininfo.ca && req.url ~ "/"
Jan 13 23:08:46 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49601 127.0.0.1 6082 Wr 200

By running cron I get:

Jan 13 23:09:06 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49608 127.0.0.1 6082 Rd auth LONGRANDOMSTRING1
Jan 13 23:09:06 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49608 127.0.0.1 6082 Wr 200 -----------------------------#012Varnish Cache CLI 1.0#012-----------------------------#012Linux,3.2.0-56-generic,x86_64,-smalloc,-smalloc,-hcritbit#012#012Type 'help' for command list.#012Type 'quit' to close CLI session.
Jan 13 23:09:06 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49608 127.0.0.1 6082 Rd status
Jan 13 23:09:06 hqdev varnishd[12371]: CLI telnet 127.0.0.1 49608 127.0.0.1 6082 Wr 200 Child in state running

So how do you "abort cache clear"?

@mgifford - there are settings on the Varnish module config.

I just found out this issue still exists in Varnish-module, and it is wiping varnish way too often. I found it out when doing a performance review for a site, when I simply ran:
varnishlog|grep "Rd ban"

Which revealed those requests:
Rd ban req.http.host ~ site.com && req.url ~ "/"

Would the patches work against Varnish 7.x-1.0-beta2?

@bibo - probably yes, but development and testing occurs on the -dev branch.

This works for me. Thanks.

This was committed at http://drupalcode.org/project/varnish.git/commit/0433ec1

We finally upgraded the module and this actually breaks any installs using VARNISH_NO_CLEAR as the default as it immediately kills all processing but there is a

if (empty($cid) && variable_get('varnish_cache_clear', 1)) {

check below and then a perfectly valid else that can never be reached.

@marcingy - This was long-closed and has a commit associated with it... If there's a problem now I'd recommend opening a new ticket.

...and please post a link to it here or tag it as related so we can follow if we wish.