Avoid full cache clear whenever a rules component or reaction rule is edited

With the patch in #9, if I try to add a loop to an action set, I get a fatal error "Call to undefined function rules_rule_build_action_info()"

1. +++ b/includes/rules.core.inc
   @@ -339,7 +339,14 @@ abstract class RulesExtendable extends FacesExtendable {
   +      $this->cache += rules_fetch_data($this->hook);
   

   How can this happen / be necessary? The cache is just initialized above, does that mean the cache is built "broken"?

2. +++ b/includes/rules.core.inc
   @@ -1064,11 +1071,32 @@ abstract class RulesPlugin extends RulesExtendable {
   +  public function getCid(){
   

   Good addition. Let's better name this getCacheId() though.

3. +++ b/includes/rules.core.inc
   @@ -1064,11 +1071,32 @@ abstract class RulesPlugin extends RulesExtendable {
   +      // Clear cache of this child element if this isn't a rebuild.
   +      if (empty($this->is_rebuild)) {
   

   Why would we skip this if it is a rebuild? That does not say there are no changes.

4. +++ b/includes/rules.core.inc
   @@ -1113,20 +1147,31 @@ abstract class RulesPlugin extends RulesExtendable {
   +          if (!empty($original) && $this->componentVariables() != $original->componentVariables()) {
   

   When adding a new one, we need to rebuild caches as well or the new action won't be available.

5. +++ b/includes/rules.core.inc
   @@ -1113,20 +1147,31 @@ abstract class RulesPlugin extends RulesExtendable {
   +          // Make sure any remove event is updated too.
   +          if (!empty($original)) {
   +            foreach (array_diff($original->events, $this->events()) as $event_name) {
   

   Let's better aggregate event names first and then rebuild caches for all of them. This should make the code a bit simpler.

6. +++ b/modules/rules_core.rules.inc
   @@ -229,49 +229,85 @@ function rules_rules_core_data_processor_info() {
   +function rules_rule_build_condition_info(RulesConditionInterface $rules_config) {
   +  static $defaults;
   +  // Ensures that t() is called just once for this.
   +  if (!$defaults) {
   

   What's that about? Why would it be called more often?

7. +++ b/rules.module
   @@ -388,8 +401,24 @@ function _rules_rebuild_component_cache() {
   +        $data_cache['action_info']['component_' . $component->name] = rules_rule_build_action_info($component);
   +        $data_cache['action_info'] += rules_discover_plugins('RulesActionHandlerInterface');
   +        drupal_alter('rules_action_info', $data_cache['action_info']);
   

   Uhm, fuddling around with the built action/condition info cache seems to be dangerous, i.e. could quickly lead to obscure bugs.
   Couldn't we just improve it to rebuild whole condition/action info caches when not more is necessary? Otherwise, if you need to save the cache rebuild I could see us adding an opt-out of cache-rebuilding when you add a new component?

8. +++ b/rules.module
   @@ -420,13 +449,48 @@ function rules_flush_caches() {
   +function rules_is_event_configured($event_name, $reset = FALSE) {
   +  $rules_event_whitelist = rules_get_cache('rules_event_whitelist');
   +  if ($rules_event_whitelist === FALSE || $reset) {
   

   Hm, that adds quite a few function call for *every* rules_invoke_event() what's bad. Also, I'm not sure about adding the separate cache vs the previous variable cache - why is that better?
   Reading #2042851: PDOException: SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'rules_event_whitelist' for key 'PRIMARY - I see, but could we handle that over there? One issue by one? :-)

Looks like it needs a re-roll. I'll try and do so.

patching file includes/rules.core.inc
patching file includes/rules.plugins.inc
patching file modules/rules_core.rules.inc
patching file rules.module
Hunk #1 FAILED at 329.
Hunk #2 succeeded at 396 (offset 22 lines).
Hunk #3 succeeded at 426 (offset 22 lines).
Hunk #4 succeeded at 474 (offset 22 lines).
Hunk #5 FAILED at 1049.
Hunk #6 FAILED at 1082.
3 out of 6 hunks FAILED -- saving rejects to file rules.module.rej

Hmm. I gave it a try but it looks like a different approach was taken instead of static cache to use a $conf instead.

FWIW, here's a the reroll, though I'm not totally sure I got it correct, @das-peter thoughts?

Linking up a different approach to the same problem

@das-peter This needed another re-roll. Again not sure if I got the conflicts merged correctly. Could you have a peek here? Or maybe this is not needed anymore?

Having a closer look at this one, here are my thoughts:

• Updating action/condition info with the new component data is trouble-some as this cache is per language. We could do so for the current language, but then the problem still persists for the other languages.
• Rebuild component cache per component I think would be a good improvement - that way the operation will take less time and the lock will be more granular. I'd not updated it immediately though, such that the update operation of the cache triggers the proper lock.
• As the patch starts, same we can do for events: Rebuild them separately, thus resulting in more fine-granular locking. Looking up the locks one some life sites, most of them are in rules_event_whitelist - thus splitting that rebuild process up should help a lot.
• Related, I checked the queries for loading the uncached configuration and noted that the loading active reaction rules is not indexed (no index on 'active'). So we can improve this a bit also.
• Finally, rules_get_cache() locking has a re-duced wait time of 10 seconds but starts to duplicate efforts after it - what is counter-productive if the rebuild is not finished in time. Let's increase waiting time to the default of 30s.

However, all of this improvements do not address this issue - still, a component change including variables, or an addition triggers clearing of the complete rules cache. We can easily improve this to only clear the rules 'data' cache + the cache of the component changed, once we have done the improvements above. Hopefully, that will be enough to solve the problems. If not, I think we should split up the rules 'data' cache monolith into separate chunks per (condition_info, action_info, ..) which can be built + cleared separately by converting it to a DrupalCacheArray (as it was done with the ThemeRegistry).

Created #2650886: Reduce the likeliness of cache rebuilding locks for the improvements not necessarily related to this and leaving this issue open for

>We can easily improve this to only clear the rules 'data' cache + the cache of the component changed, once we have done the improvements above.

ok, I worked over the patch to make it work as described before (points 1+2) from #20 + #21.

>I'd not updated it immediately though, such that the update operation of the cache triggers the proper lock.
I figured it should be fine to update immediately and did so for events + components. As there should be old, active cache entries that are used meanwhile, there is no need for a lock while building the cache entry when updating per rule-save.

Patch attached. In my tests it worked fine.

I've been using this patch for a while, so far so good. +1 but could use a last code-review before going in

Patch just needed a little tweak/re-roll to apply against the current HEAD. I haven't reviewed this yet - this is just a re-roll to bring it up to date.

The testbot shows that the patch introduces two coding standards errors, so here is a re-roll with those two fixed:

Thanks for the re-roll @TR, I've reapplied it.

So far it's on two production sites and no issues to speak of

hi
i try #17 and #19 with phpstorm and patch has some chunk issues

@delacosta45:

i try #17 and #19 with phpstorm and patch has some chunk issues

#17 is from the year 2014 and #19 is from 2015 - of course they don't apply anymore! I really don't understand the purpose of your post. #17 and #19 are obsolete patches, so why bother testing them?

Also, those are NOT the patches that we care about and those patches will NOT be committed. Fixes to these patches were made in #23, and that patch was re-rolled and improved in #26 and #27.

The patch under consideration is #27, which is more than a year old and needs a re-roll to apply against the current Rules HEAD.

It would be helpful if you could re-roll #27 and try it yourself, then post the new patch and your review here.

@TR

hi ok .. as those patch were not hidden and doesn't have the same naming i though i had to apply the one before the other.

Applying #27 directly applied successfully .

Re-roll of #27.

Other patches hidden to avoid the mistake like #32

Do we need to consider this for 8.x? I think the caching is done in a completely different way, and it is not a problem at 8.x but just wanted to ask the question.

No, none of this should be considered for D8. Caching in D8 is mostly a function of core. If someone provides specific tests/use-cases to demonstrate that caching in D8 can be improved, that will require a completely different solution and should be handled in a separate issue. At this time I have seen no indication that caching is currently a problem in D8.

The only thing bothering me about this patch is that it has the potential to be disruptive to the >200,000 users of D7 Rules. The D7 version of Rules is very stable, and we ought to take great pains to do nothing to break sites that have been using Rules for many years. While it's great we have a patch for people who DO need it, I question whether this should be pushed into the codebase to be used by people who DON'T need it. A set of test cases would go a long way to prove that this won't adversely affect existing users, especially since our D7 version doesn't have very good coverage to being with.