Regression: View pane rendering misuses and floods cache

This is impacting one of my sites too. The html on the page isn't excessive but the cache table still grows endlessly without intervention it's just not as fast. New cache entries get created on every anonymous page view, even when the content of the page doesn't change. The entries do not have an expiration so they do not get cleared during cron and a manual cache clear is necessary to get rid of them.

The patch solves the problem for me.

This cache flooding bug introduced in #1910608: Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) is part of both 1.11 and now 1.12 released two days ago.

Leeteq, thank you for reporting the issue, but please don't change the version number. It indicates the version the bug might be fixed in -- which is almost always the 'dev' version -- not the version the bug is found. Please use the comment text to indicate the 'found in' version number.

The cache table gets flooded by entries of the cid pattern "view_panel_pane_[DOM-ID]". This is because on each page request the DOM-ID of each Views panel pane changes, which leads to very extensive memory consumption. One of my websites makes intensive use of Views panel panes on its Homepage and also on further Landing pages. Its mySQL-Database gets blown up by > 30 GB within a few days. From that viewpoint caching of Views panel panes doesn't make sense at all. I'm in favour of completely removing this "feature".

+++ b/views_content/plugins/views/views_content_plugin_display_panel_pane.inc
@@ -48,7 +48,7 @@ class views_content_plugin_display_panel_pane extends views_plugin_display {
+  function set_pane_conf($conf = array(), $set_cache = FALSE) {

This functionn needs a docblock

@darrenwh That may be true but we're not adding the function in this patch or trying to fix everything about it. This is a major problem making a serious impact on sites so I think we should keep it focused on that.

It might be worth opening an issue about adding docblocks to functions that don't have them yet.

@jmuzz the patch in #2 does its job well as a workaround for sites in pain, but we need to understand the cause problem before proper progress can be made -- hence the following:

All: I am struggling to reproduce it on a clean site; that is just installed, with the minimum of other contrib modules enabled. It's probably just me not understanding, but I would be grateful if anyone who does see the problem for real can determine a minimal repro method. So far I've site-installed a D7 site with ctools, views, panels enabled, and devel-generated some content, and set up a view and various panel pages.. but I have not yet seen the views_panel_pane_* cache items set.

I attach a patch in which I have attempted to remove the cache-get / cache-set code but leave the remaining elements unchanged.

THIS IS NOT FOR PRODUCTION USE!! I am not even sure it "works".

I thought it might be a useful starting point while I and others try to get to the bottom of the problem (which, loosely stated, is "what is going on here, and what was supposed to happen").

As an example of impact my cache database table grew by 7GB through the weekend because of these cache values.

@rivimey Try using use Panels, Panelizer(?), Views Content Panes, create a views content pane and to panel/panelizer then get some traffic those pages. see the entries in your cache table.

Cache table needs to have expire values for View_panel_pane values is the answer. The recent patch Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) simply removed the Cache Temporary setting that was allowing this to self-manage. Now these entries are only cleared with a "Clear Cache All" because no expire value is set. This bloats the database between maintenance windows.

Can we revert the patch applied with Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) and start over? Maybe fix by providing a better expire value that is not equal to infinite.

To reproduce this, install a clean d7 site, enable ctools, views, panels, panels_everywhere, pages. Create a new view with a content pane inside of it. Save. Enter admin/structure/pages and enable the default site template of panels_everywhere. Create a new variant and hit save. Go to the content tab and add new content to any panel region, e.g. "Center". Under "View panes" choose your newly created view. Save. View the Homepage and make sure your view is being displayed. View your database table "cache" – et voilà – you can see your first view_panel_pane_[DOM-ID] entry. Re-load your Homepage. The second entry appears in the cache table, and so on …

This patch leaves caching but manages the growth by expiring entries.

Sorry think I was a little misguided on the various patches, also not to overlook all the time spent on this issue previously.

To help with unnecessary database growth right now, lets use last change requested by @rklawson for views_content_plugin_display_panel_pane.inc 1910608_48. We might tweak a little with a cache value of say an hour. This timing would help combat frequent system_cron runs when using CACHE_TEMPORARY while still allowing cache table work and expire values to clean itself up.

Certainly still want to look at why these Cache entries grow with each page load and whether that is even necessary.

Seems reasonable to put a time on the caches. That would fix the major impact of the problem and still allow the rest to be worked out after without damaging anything. I don't see how that could fail to qualify as proper progress.

It is still making a new cache entry with every page request instead of using those cache entries, but that could go in a followup issue unless it can be easily fixed.

This patch removes cache completely

@rivimey - Your patch to remove cache applies well and tested with no cache additional entries generated. This attached patch is equivalent to what you provided with a fix to the watchdog lines.

Cache entries rely on dom_id which is never found in view_panel_panes (at least not now). So the dom_id are always random and will only every be generated once and never used again. It would be nice if we can build a better way to build the right dom_id to get cache working properly. Maybe a combination of view name, arguments, context, node, revision, delta. Ideally something that would be the same for repeat page visits then cache could be utilized and would not grow.

$this->view->dom_id = !empty($this->view->dom_id) ? $this->view->dom_id : md5($this->view->name . REQUEST_TIME . rand());

Patch #16 applied on my test environment leads to the following behavior:
No more cache entries of rendered view_panel_panes. On each page request 2 watchdog entries per views panel pane are created.

@whthat thanks for your assistance in this. My patch was based on the thought that a cache using a random identifier wasn't very useful, because we'd not be able to re-fetch the same one later. The only way to refetch the entry would be if the identifier was retained in memory, and I was unable to determine if that was happening or not. Hence my 'not for production' not.

Regarding "Maybe a combination of view name, arguments, context, node, revision, delta.", if this is the path taken, should we consider including the user-id in there too? I'm unsure if there is a risk of leaking information from one user to another.

My worry about deleting the cache code is that I can only think (given the circumstances of the original code) that the cache was not really being used as a cache -- that is, storing regeneratable information -- but as a short term store of non-regen information. Again, I was unable to determine if that was the case or not, which worries me. The original change (1910608) was all about remembering panel settings, even across ajax calls, and I can see that storing that in the cache, identified by dom_id, makes sense. The downside is that we're now using a random-id, which doesn't make sense (to me, at least).

Many thanks for the work and testing you've done.. I would greatly appreciate some simpletests for this area if you can manage that, or even some explicit test cases someone else could write tests around.

@kreatIL If you wish, deleting the two lines calling watchdog from the **result of applying** the patch would be safe, as would replacing WATCHDOG_WARNING with WATCHDOG_INFO. They are there because we are still exploring the nature of the problem. It would be useful to note in this issue if you ever see a case where only one call happened.

It would also be useful to note roughly what proportion of watchdog entries have an empty or very short pane config in those watchdog entries compared to the number that have an extended (multi-line) config.

This bug is contained in production code since version 7.x-1.11, not only in dev as the original issue description suggested.

Corrected some spelling

This patch is #16 with:

• Removed debugging watchdog() calls.
• Removed views_content_plugin_display_panel_pane::set_pane_conf()’s unused $set_cache argument.

I’ll be testing this out on Drupal.org since this has been abusing our disks.

Adding an actual patch.

This has been deployed on www.drupal.org for a few minutes with no noticeable performance or functionality impact. And we won’t be forced to clear the cache every few days.

@drumm, great that you've been able to help.

The set_cache arg had been left in there because I was not sure if other modules had called that function and so removing it would cause php errors. I am not aware of such calls, but there is an awful lot of custom module code out there...

My main concern (expressed in #18, is that while the change posted fixes the cache issue, it breaks something else.

If we regard that possibility as being less urgent/important than getting the patch into production (which seems possible), then lets get it out there. Otherwise, we need some more tests writing... I'm trying to get that initiative going but there's a lot of code and few people... :)

I'll propose but not RTBC for the moment as my concerns still persist.

The $set_cache argument was added October 15, so there's relatively little time it might have been implemented. www.drupal.org’s codebase had no other references to it.

Something is weird, #24 patch and previous cache removal patches appears to be breaking views using a views_field_view field. The child view that we referenced is built as a Content pane, which is likely were issues arise. We have had this setup running for a couple of years now.

Change priority... seems appropriate.

#31 yes the cache removal patche #24 & #16 were causing it, vanilla 7.x-1.12 showed the views just fine.

This patch fixed my problems with views_field_view fields found in #30. Changes from #24 include: Function pane_process_conf needs to return if the conf is found empty and it does not need to include the same commands as set_pane_conf.

Patch using a cache context as described in #16 and #18. This creates one cache entry per user id that visits a page with the panel on it. Multiple visits by the same user do not create multiple cache entries. Tested using the steps as described in #12.

Added language to the cache key.

Come to think of it: Not sure if current_path is relevant in the cache key? since the display an arguments are in the key already... patch without the path in the cache key.

Jelle_S, great work. Had a brief look at the patch, and wonder if perhaps a crc32() of the serialize()d value in the cache key might be worthwhile? Just to prevent it getting too long when there's lots of args? I think the max safe length of a cache key is likely around 250 chars (default schema=255, memcached similarly).

@rivimey true, but the cache key is md5()d, so it'll always be 32 chars to my knowledge, so i don't think that'll be an issue.

Jelle_S - sorry - missed that. :blush:

@Jelle_S this tests out well for me. Love that cache is in use, with Dom_id's being reproduce-able and cache entries are not being duplicated with less uncontrolled growth between Cache Clear All.

This patch uses everything from #37's dom_id hash then includes:

• Cache table location change from "cache" to "cache_views", similar to other views cache locations
• Changed "_" delimiter to ":" between cache name and dom_id, similar to other implementations of cache CID's
• Removed call to set_pane_conf($conf) inside of function pane_process_conf -> if (!$conf), as it would not be necessary to rewrite cache if it just located it

On the fence as to whether we should provide an expire value to the cache, now that cache would be working for us it doesn't seem necessary.

Good stuff!

I think it would be good to rename 'dom_id' as it really isn't anything to do with the DOM now.

1. +++ b/views_content/plugins/views/views_content_plugin_display_panel_pane.inc
   @@ -50,9 +50,13 @@ class views_content_plugin_display_panel_pane extends views_plugin_display {
   +      $this->view->dom_id = md5($this->view->name . '_' . $this->view->current_display . '_' . serialize($this->view->args) . '_'  . $language->language . '_' . $user->uid);
   

   Line too long; could it be split up (perhaps at the '.'?)

2. +++ b/views_content/plugins/views/views_content_plugin_display_panel_pane.inc
   @@ -426,13 +430,12 @@ class views_content_plugin_display_panel_pane extends views_plugin_display {
   +      if (!empty($this->view->dom_id) && ($cache = cache_get('view_panel_pane:' . $this->view->dom_id, 'cache_views'))) {
   

   Line too long.

@rivimey thank you for the recommendations in #42 they are included in this updated patch, no additional functionality changes:

• dom_id to cid, now matches the database field name which represents cache id
• shortend lines

Updated #44 to pass syntax test

whthat, my preference would be for patch #44 rather than #46; I suspect the test fail was the spaces at end of lines, which I think git has been told to barf on.

Patch failure was due to a bug in drupalci.

TY, Resubmitting #44 without extra spaces.

If this one fails I probably need some help.

+++ b/views_content/plugins/views/views_content_plugin_display_panel_pane.inc
@@ -425,14 +434,14 @@ class views_content_plugin_display_panel_pane extends views_plugin_display {
+      // See if cache id has been cached and get conf from there.
+      if (!empty($this->view->cid)
+        && ($cache = cache_get($this->view->cid, 'cache_views'))) {

I was wondering, since the cache never gets cleared by the code, and there is no expire time on it, it only gets cleared by manual cache clears. What if the config ($conf variable) changes? We would get it from cache here, but it wouldn't be up to date?

I am not as pessimistic as Axel: I think we can make this code work properly.

The point about $conf changing is real, though. I think that if a check is introduced such that when set_pane_conf is called the new conf is compared to the old, and if different the stored cid is nulled out, that would force the conf to be re-stored (overwriting the old cache entry because now the cid is deterministic). Including a cache expire value is I believe a good thing. It should be relatively long -- an hour? -- but having it will ensure that entries don't accumulate over time if e.g. view args or similar change and leave old entries orphaned.

I do agree with Axel that we are at very least not using the cache in an expected way, so I think this should be noted in the comments near cache_set and cache_get calls, so that others don't have to discover this. However the alternative to using cache_set/get would be to create a new db table, new code to manipulate it, and so new potential bugs, so I don't feel it is appropriate to go down that path at this point.

Would it help to use a new cache table -- i.e. not 'cache_views' but 'cache_panel_conf' or similar? It would perhaps make it easier to track down problems?

As someone who got paged by Drupal.org’s disks filling up at odd times over the holiday - I’d like to see a quick fix go in, even if it is reverting, and a new release. The old issue or a followup can always be opened to fix #1910608: Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) again.

#52 ++
#54 ++
Reverting would quickly fix the issue on the many production sites out there. And it should give you enough time to find an alternative solution for the root problem.

Reverting is fine, but this is coming from someone who did not experience the original Ajax settings issue. I could be easily outranked in need, skill or understanding here. So if we do revert I ask that we restart this patch as to not completely throw away the three years of progress and new developments with regression. With an emphasis to fully understand the patch/component elements in order to continue to provide top tier solutions.

Since the commit has not been reverted yet, I thought it wouldn't hurt continuing work on this patch. I took another stab at writing a patch that fixes the problem without re-introducing the original bug (ajax).

This patch uses the flow described in #53.

So if we do revert I ask that we restart this patch as to not completely throw away the three years of progress and new developments with regression.

The work certainly isn’t being thrown away if reverted; it is reset back to needs work/review. Regardless, there’s more work that needs to go into the followup issues that can continue regardless of whether the maintainers get a revert committed or not.

I deployed #60 to www.drupal.org - the number of cache entries is still stable, and I'm not aware of any regressions affecting us.

I also deploy #60 on high traffic website, before my cache table was 25GB now its just few MB and everything looks stable.

#60 runs just fine here

#60 runs fine both on a testing environment and a production site maintained by me. No extra cache entries.

I've reverted #1910608: Ajax + Allow settings: Allowed settings lost on ajax (exposed forms/pager) and will add comments in the new issue #2843333: Fix the Ajax settings fix's broken cache usage to see if we can move that forward.

Since the last fix has been in the last release, I'd prefer if we get the Ajax fix in before the next release. however, getting this committed at least allows folks to use the head version of ctools until that is done.