Follow-up to properly fix #996236: drupal_flush_all_caches() does not clear entity info cache (stop-gap fix for D7)

Problem

  • Data rebuild operations happening during drupal_flush_all_caches() act on stale/outdated data of other modules and subsystems.
  • hook_flush_caches() implementations rebuild data on stale caches. Then they return caches to flush, which may contain the data that was just rebuilt.

Goal

  • Introduce hook_rebuild() as unique rebuild operation.
  • Properly separate cache flushing and static variable resets from rebuild operations.

Details

  • drupal_flush_all_caches() clears caches, but also issues several kind of rebuilds.
  • The menu is rebuilt and module provided configuration-alike data structures (such as node types) are rebuilt.
  • There are many interdependencies between the caches and rebuilds.
  • Rebuilds may be based on other stale/old/invalid cached data, which hasn't been cleared yet.
  • In turn, rebuilt data can be entirely bogus or incomplete.

Proposed solution

  1. Clean environment: Flush all database caches first. (hook_cache_flush())
  2. Clean environment: Reset all static caches.
  3. Trigger module rebuilds. Guaranteed to be based on clean+fresh data. (hook_rebuild())
  4. Rebuild the menu router. Guaranteed to contain most current data.
  5. Clear page, JS, and CSS caches. As an attempt to still serve cached pages during the entire operation.

API changes

  • hook_flush_caches() is renamed to hook_cache_flush() to ensure that modules are only clearing caches.
  • hook_rebuild() is added as dedicated hook for rebuilding operations.
  • menu_rebuild() (in menu.inc) is renamed to menu_router_rebuild(), since the name clashes with hook_rebuild() when Menu module is installed.

Notes

  • Some other D8 issue wants to introduce hook_cache_bins(). That could be renamed into hook_cache_info() and it would cover the 90% use-case of hook_cache_flush() here (returning names of cache bins that need to be flushed).
  • There's a long-time desire to split menu.inc into router.inc for the router system parts. So menu_router_rebuild() might be renamed into router_rebuild() later. The new Symfony kernel might make that obsolete though, we'll see.
  • The new configuration system might make some implementations of hook_rebuild() obsolete (those that attempt to synchronize active configuration with code definitions, such as node_types_rebuild()). To be investigated later, doesn't matter for this issue.
  • This patch unintentionally speeds up cron runs. Cron invokes hook_flush_caches() but only wants to flush caches. Many of the current implementations are performing additional rebuild operations (e.g., Block module).
CommentFileSizeAuthor
#66 drupal8.flush-rebuild.66.patch38.86 KBsun
#61 drupal8.flush-rebuild.61.patch40.25 KBsun
#61 interdiff.txt503 bytessun
#59 drupal8.flush-rebuild.59.patch40.16 KBsun
#59 interdiff.txt1001 bytessun
#57 drupal8.flush-rebuild.57.patch39.3 KBsun
#57 interdiff.txt1.51 KBsun
#56 1404198-55-ffs.patch39.45 KBAnonymous (not verified)
#50 1404198-50.patch37.83 KBAnonymous (not verified)
#49 1404198-49.patch37.43 KBAnonymous (not verified)
#47 1404198_47_interdiff.txt753 byteschx
#47 1404198_47.patch37.92 KBchx
#46 drupal8.flush-rebuild.46.patch37.75 KBsun
#44 drupal8.flush-rebuild.43.patch37.6 KBsun
#42 drupal8.flush-rebuild.42.patch37.57 KBsun
#42 interdiff.txt14.19 KBsun
#39 drupal8.flush-rebuild.39.patch23.4 KBsun
#39 interdiff.txt3.88 KBsun
#36 drupal8.flush-rebuild.36.patch22.19 KBsun
#36 interdiff.txt2.07 KBsun
#35 drupal8.flush-rebuild.35.patch22.19 KBsun
#35 interdiff.txt4.92 KBsun
#33 drupal8.flush-rebuild.32.patch22.14 KBsun
#30 drupal8.flush-rebuild.30.patch22.13 KBsun
#28 drupal8.flush-rebuild.28.patch21.04 KBsun
#25 drupal8.flush-rebuild.25.patch15.38 KBsun
#23 drupal8.flush-rebuild.23.without-install.core_.patch12.45 KBsun
#23 drupal8.flush-rebuild.23.patch12.99 KBsun
#16 drupal8.flush-rebuild.16.patch12.95 KBsun
#9 drupal_8_flush_caches.patch12.97 KBfago

Comments

fago’s picture

fago
Primary language German
Location Vienna
commented

I don't think two simple rebuild and cache clear stages are going to suffice, as we have multiple different kinds of rebuilds in our process. First off we've underlying rebuilds for applying module provided configuration (read: exportables), then we've something like menu_rebuild(). However, rebuilding the menu basically requires most caches to be updated for changes to take affect, while rebuilds for applying module provided configuration are really foundational and need to happen before clearing caches, so caches are updated afterwards.

Thus, I think we should
1. Rebuild the registry
2. Apply module provided configuration (node types, system info, ..) ---> General rebuild phase.
3. Clear *all* caches, including all statics so changes take affect afterwards. ---> General cache clear phase.
4. Rebuild the menu, what's going to populate lots of caches again.

Then, we should make sure no module calls menu_rebuild() in an API function, but makes use of that:

variable_set('menu_rebuild_needed', TRUE);

instead. Maybe we could improve that by having menu_rebuild() to default to this behavior.

Idea:
Using the "menu_rebuild_needed" flag, makes menu_rebuild() work like any other permanent cache flush. Thus, we could even don't bother with statics and just make sure all permanent caches are cleared in phase 3.
However, I guess having all statics cleared after drupal_flush_all_caches() would be a desired behavior anyway.

amateescu’s picture

amateescu commented

Possibly related: #1387776: Move some rebuilds outside of Drupal

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: clean-up and fix clearing caches » Separate database cache clearing from static cache clearing and data structure rebuilding
Issue tags: +API change, +API clean-up

I partially disagree with the proposed order.

To clarify what you described in #1, we have:

  1. Database cache clears (entity info, node types, languages, field data, module_implements, theme registry, etc.)

    Unless respective API functions have a primed static cache, the database cache is re-loaded.

  2. Static cache clears (entity info, entity controllers, element info, registry, module data, theme registry, etc.)

    Without primed static cache, respective API functions attempt to re-load from database cache. Otherwise, recollect/reconcile/rebuild from scratch.

  3. System components and extensions update (class registry, module data, theme data)

    Synchronizing the system's knowledge about existing classes, modules, and themes with actual filesystem data.

  4. Data structure and configuration rebuilds (node types, features, block rehashing, actions, etc.)

    Synchronizing stored data in database with code changes. Typically priming static caches with results again. Also, these can have dependencies on rebuilds of other modules - typically following module dependencies (as declared in .info files).

  5. Essential core subsystem rebuilds (menu router)

    Final update of router data to match new system state.

And that's basically also what I'd precisely envision for dfac() - to repeat:

  1. hook_flush_caches(): Flush all database caches and static caches you have, so we can continue from a clean slate. (Static caches could be taken over by dfac().)
  2. registry_rebuild() (implying system_rebuild_module_data()) + system_rebuild_theme_data(): Hard-coded update of class and extension data.
  3. hook_rebuild(): Rebuild whatever data you have that may be based on code.
  4. menu_build(): Hard-coded rebuild of the router, so following executed code and requests know about obsolete and new stuff.
  5. cache('page')->flush(): As a special exception, intentionally flush the page cache last, so any requests hammering the site during dfac() can potentially still be served.

Notes:

  • Database and static caches really have to be flushed first, or there's no way to really guarantee that we're running from a clean slate and this entire change wouldn't resolve anything.
  • Unlike other hooks, hook_rebuild() would ideally be invoked in the order of module dependencies.
  • hook_rebuild() implementations manually calling into menu_build() are broken code and we do not babysit broken code.
  • hook_menu() implementations do not manually call into any rebuild functions.
  • Wrap that entire dfac() into a locked operation, so cache flushes and rebuilds are not fucked up with suddenly primed data.
fago’s picture

fago
Primary language German
Location Vienna
commented

Wrap that entire dfac() into a locked operation, so cache flushes and rebuilds are not fucked up with suddenly primed data.

How would you like do that? This would mean we'd have to look the whole system, so no cache can get initialized in-between. However, I think if we'd have a solid order of steps with each step being locked, we should be already safe and minimize the down-time.

hook_rebuild() implementations manually calling into menu_build() are broken code and we do not babysit broken code.
hook_menu() implementations do not manually call into any rebuild functions.
5. cache('page')->flush(): As a special exception, intentionally flush the page cache last, so any requests hammering the site during dfac() can potentially still be served.

Agreed.

Unlike other hooks, hook_rebuild() would ideally be invoked in the order of module dependencies.

Makes sense. However, it sounds quite complicated and I can't think of any case requiring that right now. So, going with regular module weights and/or module controlled ordering via hook_module_implements_alter() should do it for now? We can still go and improve it once the need for it really comes up.

Database and static caches really have to be flushed first, or there's no way to really guarantee that we're running from a clean slate and this entire change wouldn't resolve anything.

I don't think so any more. Point 3) (System components and extensions update) does not depend on 1) and 2) (permanent and static cache clears) in any way, however there are dependencies the other way round. Thus I think point 3) should run first.

Similarly, I think point 4) (Data structure and configuration rebuilds) usually won't require updated caches neither, as reading configuration files and synching them into the db mostly depends on 3). Furthermore, rebuilding items might issue quite some changes and cache-clears - thus flushing all caches afterwards would save us from quite some repetitive cache flushes. Next, rebuilt items won't be properly reflected in caches if the modules in question do not properly clear all affected caches, what sometimes is not even properly possible due to cache-dependency-chains (cache A depends on cache B, but modules clears only B so A stays outdated). Only doing a full-cache clear after rebuilding would guarantee completely fresh caches.

Thus, I'd suggest the following order:

  1. registry_rebuild() (implying system_rebuild_module_data()) + system_rebuild_theme_data(): Hard-coded update of class and extension data.
  2. hook_rebuild(): Rebuild whatever data you have that may be based on code.
  3. hook_flush_caches(): Flush all database caches and static caches you have, so we can continue from a clean slate. (Static caches could be taken over by dfac().)
  4. menu_rebuild(): Hard-coded rebuild of the router, so following executed code and requests know about obsolete and new stuff.
  5. cache('page')->flush(): As a special exception, intentionally flush the page cache last, so any requests hammering the site during dfac() can potentially still be served.

Thinking about it again, the best solution probably boils down to the rebuild-operation one thinks of. Rebuild operations that sync rather foundational, defined configuration items (views, content types, rules, ...) should run before cache clears to make sure caches are properly flushed once, but the other way round rebuilding data that depends on quite some other data and configuration (menu, blocks) should run last and after clearing caches.

Thus, we might even want to go with two different rebuild steps or would require the latter rebuilding steps to move to a cache-like approach: 1. Clear caches/set the rebuild flag. 2. Rebuild once on de-mand and cache results. We even have that flag in place for the menu.
However, we really cannot skip the first rebuild step without having a repetitive drupal_flush_all_caches() to force everything is updated.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This would mean we'd have to look the whole system, so no cache can get initialized in-between.

Enhancing the lock system to allow for a special, global lock doesn't sound that bad, actually.

However, I think if we'd have a solid order of steps with each step being locked, we should be already safe and minimize the down-time.

Nope. Consider:

1) After clearing caches
2) assume you need to rebuild
3) another request primes the caches again
4) call into the rebuild hooks, then
5) either nothing is rebuilt, or
6) the world ends with a nuclear disaster, fatal error.

hook_rebuild(): going with regular module weights and/or module controlled ordering via hook_module_implements_alter() should do it for now

Yeah, potentially. (It sounds like a very good idea to do though.)

4) (Data structure and configuration rebuilds) usually won't require updated caches neither, as reading configuration files and synching them into the db mostly depends on 3).

They do, heavily even. Especially this kind of data is typically cached right within the function that builds and returns it.

Even essential stuff like module_list() itself relies on caches. What we're after is a clear separation between

1) flushing any and all old data and caches, and
2) (re)building data structures and configuration

This is essential. We do not want to have any old data in these operations, which (naturally) can only be stale and outdated.

rebuilding items might issue quite some changes and cache-clears - thus flushing all caches afterwards would save us from quite some repetitive cache flushes.

Nope. Data is only rebuilt if there is no cached data in the first place. Again: We need a clear separation of stages and concepts.

A rebuild operation might set a cache after rebuilding - but that is fine, as long as we can be sure that everything was built from scratch.

rebuilt items won't be properly reflected in caches if the modules in question do not properly clear all affected caches, what sometimes is not even properly possible due to cache-dependency-chains (cache A depends on cache B, but modules clears only B so A stays outdated).

"Not properly clearing all caches" during the (first) cache flush operation of dfac() means broken code.

We do not babysit broken code, so there cannot be cache dependency chains for rebuild operations in the first place.

Only doing a full-cache clear after rebuilding would guarantee completely fresh caches.

Yes, I totally agree that all caches need to be flushed. However, caches are only created when they are built. And that's what the rebuild operation is for (though not everything is directly cached upon rebuild).

To make sure this really gets across, as I really can't stress enough on it:

You cannot reliably rebuild data structures and configuration, if there is a possibility of a function returning old/stale data somewhere in between.

Especially because of the module data dependency chains in the rebuild operations it would be utterly wrong to attempt to (re)build anything while having a chance of stale data anywhere.

All rebuild operations need to be able to rely on the fact that all caches have been cleared.

Without this essential difference in the flow, we're not going to improve anything. If there's any hook implementation deep down the rabbit hole that returns stale data from an old/outdated cache, then the data you are trying to (re)build is fucked up.

That's the exact systematic problem that #996236: drupal_flush_all_caches() does not clear entity info cache revealed.

The new paradigm has to be: Kill it. Build it.

fago’s picture

fago
Primary language German
Location Vienna
commented

@sun:

Nope. Data is only rebuilt if there is no cached data in the first place. Again: We need a clear separation of stages and concepts.

I'd not say so. I think we are not talking about the same kind of 'rebuild'. So let's try to clarify what we mean with 'rebuild':

To me 'rebuild' is an operation that prepares data, that is used afterwards. There is a difference to caches in that, that the system relies upon this built data to be there afterwards, whereby caches can be refreshed anytime they have been cleared.

Examples of that would be the registry or synching node-types to the database. If node-types have not been imported, they are just missing as the system won't be able to determine their absence.

That makes it in particular problematic as caches might require those items to have been rebuilt in order to not contain stale data. In case of caches it's easier, as you can just clear everything and caches are re-initialized just on-demand, thus automatically building in the right order. Given that, we have to rebuild *before* the complete cache clear in order for caches to be able to reflect the rebuilt data.

(on cache vs rebuilding data: I already tried to handle rebuilding exportables like a regular cache clear by doing a rebuild on-demand with exportable entities, what results entity-crud hooks being possibly triggered by a simple entity-load operation. That way quite some ugly nested hook-invocations as seen in #1267070-7: Lazy-rebuilding the defaults for exported entities might not be desirable occur, what we better avoid by handling rebuilds explicitly.)

Example:
Node types need to be rebuilt before the entity info cache is initialized. Flushing all caches first, then going in rebuild phase probably ends up with someone triggering entity-info cache building before node types can be rebuilt, and voila your entity-info is outdated.

Possible solution:
1. complete cache clear
2. rebuilds
3. complete cache clear

I do think though the first complete cache clear is not necessary for most kind of rebuilds, however I agree that we should clear module lists first (as required by the registry rebuild).

->
1. clear module caches (no dependencies)
2. rebuilds (may depend on modules being up2date)
3. complete cache clear

If you do think of rebuilds that require more caches cleared, please give some examples.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

we have to rebuild *before* the complete cache clear in order for caches to be able to reflect the rebuilt data.

Extremely simple example of how not flushing caches upfront and not being able to rely on a clean slate fails:

/**
 * Implements hook_rebuild().
 */
function mymodule_rebuild() {
  // Delete all mappings that no longer exist.
  $entity_info = entity_get_info();  // FAIL.
  $mappings = db_query('SELECT id, entity_type FROM {mymodule}')->fetchAllKeyed();
  foreach ($mappings as $id => $type) {
    if (!isset($entity_info[$type])) {
      mymodule_mapping_delete($id);
    }
  }
}

Albeit abstract, there's most likely no simpler example.

If you really want just one of plenty real-world examples that involves a plethora of cached data in statics and database, witness _block_rehash().

There's no point in flushing caches after rebuilding. You've either rebuilt everything from scratch and from a clean slate, or you did not. At the point you're no longer sure, the system is unreliable and the design of the process broken.

fago’s picture

fago
Primary language German
Location Vienna
commented

There's no point in flushing caches after rebuilding. You've either rebuilt everything from scratch and from a clean slate, or you did not. At the point you're no longer sure, the system is unreliable and the design of the process broken.

So what's with the example I've given then?:

Node types need to be rebuilt before the entity info cache is initialized. Flushing all caches first, then going in rebuild phase probably ends up with someone triggering entity-info cache building before node types can be rebuilt, and voila your entity-info is outdated.

The same holds for quite some other exportables that need to be rebuilt. To apply those exportables though, we'd require another cache clear. Thus, are you suggesting we should do it twice?

If you really want just one of plenty real-world examples that involves a plethora of cached data in statics and database, witness _block_rehash().

Indeed, that one needs to happen after all caches have been cleared. Probably best we'd just have two hooks, one before flushing caches and one afterwards. :/ Let's face it, we have real-world use-cases for both.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Active » Needs review
StatusFileSize
new drupal_8_flush_caches.patch12.97 KB

ok, here is a patch overhauling cache-clearing in 3 phases. Let's see what the test-bot says.

Patch also fixes revamps simple-tests resetAll() methods to rely more on drupal_flush_caches() - a good proof it's working right (or not). Also, fixed a test-problem in the comments fields tests where previously caches were rebuilt without the full module list being loaded.

Status: Needs review » Needs work

The last submitted patch, drupal_8_flush_caches.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs work » Needs review

>Drupal installation failed.

Cannot reproduce. :/

fago’s picture

fago
Primary language German
Location Vienna
commented
Issue tags: -API change, -API clean-up

#9: drupal_8_flush_caches.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API change, +API clean-up

The last submitted patch, drupal_8_flush_caches.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Node types need to be rebuilt before the entity info cache is initialized. Flushing all caches first, then going in rebuild phase probably ends up with someone triggering entity-info cache building before node types can be rebuilt, and voila your entity-info is outdated.

The same holds for quite some other exportables that need to be rebuilt. To apply those exportables though, we'd require another cache clear. Thus, are you suggesting we should do it twice?

The situation you are describing can only happen in the current system, the broken system we need to fix.

In the new system, it does not matter what is rebuilt first or later, and which function primes its cache(s) after rebuilding. It does not matter at all, because we're running from a clean slate and everything has been reset to NULL in the first place.

Therefore, there cannot be a wonky dependency between node types and entity info caches. If one depends on the other, that's perfectly fine: the other is rebuilt, too. It's only rebuilt once, and only needs to be rebuilt once, because the new flow and clear separation between hook_flush_caches() and hook_rebuild() ensures that everything old we knew before is forgotten, and the registry and extensions are updated before we reach the rebuild stage.

Thus, if you happen to have a rebuild that primes a cache and it doesn't contain everything that's there, then that's obviously a major bug and broken code in the module that should have returned its data when the data was rebuilt.

Your argumentation would mean that Drupal wouldn't be able to build (not rebuild) the data it requires when starting from null, nada, scratch, ground zero, no pre-existing information. That's not the case, Drupal is and must be able to do so.

What this API change needs to achieve is to essentially make dfac() the system get "back" to that null state. Only after doing so, we have a reliable system state and are able to [re]build information. (Depending on the individual implementation, it's not necessarily a "rebuild", it may just be a straight "build" (because there's no previous information anymore).)

+++ b/core/modules/node/node.module
@@ -765,6 +765,13 @@ function node_type_cache_reset() {
 /**
+ * Implements hook_flush_caches_prepare().
+ */
+function node_flush_caches_prepare() {
+  node_types_rebuild();
+}

Sorry, but this is zero improvement to the current dfac() nightmare and getting nowhere. This doesn't separate concepts at all.

fago’s picture

fago
Primary language German
Location Vienna
commented

Thus, if you happen to have a rebuild that primes a cache and it doesn't contain everything that's there, then that's obviously a major bug and broken code in the module that should have returned its data when the data was rebuilt.

I agree with you that ideally, we'd just have cache-like systems that we can flush + rebuild afterwards. However, in reality we haven't.

We have lots of such "broken code":

  • field definitions synched into the database
  • the registry
  • system date formats and types
  • node types
  • some exportables from contrib
  • module and theme information
  • the menu system

All of those are not flush-able like caches, so that they re-initialize on demand. If you just clear those data, then necessary information won't be rebuilt - it will be missing.

Let's consider the example of node types. First off there is node_types_rebuild() what is necessary to detect new/gone-away node types from modules. Then there is node_type_cache_reset(), which is about clearing the node-type cache. Clearing the cache won't help you to discover new node-types. Deleting all node-types from the db won't help you to automatically discover them either, you just loose your customizations and your non-code node types. That is why node_types_rebuild() is invoked explicitly on module enable / disable and dfac.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
Issue tags: +Framework Initiative
StatusFileSize
new drupal8.flush-rebuild.16.patch12.95 KB

Attached patch implements a clear and concise separation between cache and rebuild concepts.

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.16.patch, failed testing.

fago’s picture

fago
Primary language German
Location Vienna
commented

Meantime, I realized my proposal won't fly. Rebuilding even foundational stuff without properly cleared caches partly relies on a working system to do even this foundational basic rebuilds. However, we really should be able to recover from any wrong, incomplete or whatever caches by running dfac(). Thus, finally +1 for sun's approach even if it means some caches have to be re-freshed twice.

Ideally, I guess rebuilds should try to clear all necessary caches after rebuilding. E.g., like rebuilding node-types already triggers a menu rebuild. That said, rebuild operations should offer hooks other modules can use to clear necessary caches (= node type CRUD hooks).

 function block_flush_caches() {
+  return array('block');
+}
+
+/**
+ * Implements hook_rebuild().
+ */
+function block_rebuild() {

This requires the global lock to work, else we might end up with old blocks.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

We should figure out what's wrong with the testbot first. I'm able to successfully install Drupal with this patch applied and do not see any kind of hiccups.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Possibly related: #1213536: Non-resettable theme_get_registry() cache causes problems for non-interactive installations

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
Issue tags: -Framework Initiative, -API change, -API clean-up

#16: drupal8.flush-rebuild.16.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API change, +API clean-up

The last submitted patch, drupal8.flush-rebuild.16.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.23.patch12.99 KB
new drupal8.flush-rebuild.23.without-install.core_.patch12.45 KB

Hrm. Re-rolling this patch is painful.

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.23.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.25.patch15.38 KB

I'm not able to reproduce the installation test failure locally -- neither through the UI, nor via run-tests.sh.

Regardless of that, trying whether further adjustments to install_finished() make any difference.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work 
+++ b/core/includes/common.inc
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ * @todo Rename hook_flush_caches() to enforce developers to re-implement it
+ *   according to this contract.

I think this is a good idea. Not sure whether to do it in this patch or in a follow-up issue though?

+++ b/core/includes/common.inc
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+ * @todo Make this obsolete:
+ * Note that after clearing all static caches also the theme is not initialized
+ * any more and any possible previously added Javascript or CSS is gone.

I've no idea how to resolve this. (this is actually the same comment as the @todo in install_finished())

+++ b/core/includes/common.inc
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+  // Clear all non-drupal_static() static caches.
+  // @todo Grep entire core for statics that need to be reset and can be reset,
+  //   and reset them here, or for modules, in hook_flush_caches().

There are actually no non-drupal statics to reset currently. But it would be good to keep the comment (without @todo), in order to state that - if regular statics need to be reset at some point - then they should be reset here.

+++ b/core/includes/install.core.inc
@@ -1571,28 +1571,34 @@ function install_import_translations_remaining(&$install_state) {
+  // Re-initialize the maintenance theme.
+  // @todo drupal_flush_all_caches() resets everything, and afterwards rebuilds
+  //   various data structures, among which menu_rebuild() invokes hook_menu()
+  //   implementations, which may trigger a theme initialization (e.g., Block
+  //   module defines separate menu items per theme).
+  unset($GLOBALS['theme']);
+  drupal_maintenance_theme();

This @todo is actually wrong. It should instead clarify that this is needed, because the drupal statics of list_themes() and drupal_add_js/css() are completely reset by dfac(), so the install finished page would only contain the bare/raw text output without any maintenance theme styling.

+++ b/core/modules/system/system.module
@@ -2531,6 +2531,7 @@ function _system_update_bootstrap_status() {
+  // @todo This the caller's job and not always necessary.
   system_list_reset();

Let's fix the calling functions accordingly as part of this patch.

aspilicious’s picture

aspilicious commented
Status: Needs work » Needs review 
+++ b/core/includes/common.incundefined
@@ -7289,50 +7289,89 @@ function drupal_implode_tags($tags) {
+  // Rebuild all information based on new module data.
+  module_invoke_all('rebuild');
 
....

-  }
+  // Lastly, rebuild the menu router based on all rebuilt data.
+  menu_rebuild();

Doesn't this run menu_rebuild twice?

-24 days to next Drupal core point release.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.flush-rebuild.28.patch21.04 KB

@aspilicious: Nope, menu_rebuild() is not a hook implementation of hook_rebuild().

Resolved all issues in #26:

  1. Renamed hook_flush_caches() into hook_cache_flush(). Required to ensure that any kind of rebuild operations in existing hook_flush_caches() implementations are appropriately migrated into hook_rebuild().
  2. Kept and clarified the @todo about theme/js/css statics being reset. Removed the corresponding @todo from install_finished().
  3. Removed the system_list_reset() from _system_update_bootstrap_status().

    Note: If this patch fails, then we need a system_list_reset() after invoking it in dfac(). (or re-order the invocation)

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.28.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.30.patch22.13 KB

whoopsie -- didn't catch all instances of 'flush_caches' throughout core ;)

+++ b/core/includes/common.inc
@@ -7289,50 +7289,88 @@ function drupal_implode_tags($tags) {
+ * - Clears page and asset/support file caches (last, in order to serve requests
+ *   from cache during re-initialization, if possible) @todo While a good idea,
+ *   this breaks the contract and doesn't work out, since cache_bootstrap would
+ *   also have to be retained in order to serve pages from cache. :(

whoopsie -- missed the inline @todo here ;)

This patch is RTBC from my perspective.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Assigned: Unassigned » sun
hefox’s picture

hefox commented

Skim read the issue, so may be missing out anyone bringing this up, but perhaps a less generic hook then hook_rebuild would be good?

Feels like it could cause some confusion when newer people see it vs. registry_rebuild, menu_rebuild, etc. and also limits the namespace (can't have a module named registry, menu, etc.). Also, seems like a possibility of mistakenly naming a function [module]_rebuild and accidentally implementing that hook XD!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.flush-rebuild.32.patch22.14 KB

Replaced "database caches" with "persistent caches."

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Needs work

i really like this patch. a couple of smallish things:

- i think 'database caches' reference should be replaced with 'persistent caches'

- the moving around of _system_update_bootstrap_status() could do with some comments, reading the patch it's unclear why that's important

- i think we should implement system_cache_flush() and return those bins, rather than special casing 'core' bins

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt4.92 KB
new drupal8.flush-rebuild.35.patch22.19 KB

Based on #34:

1) renamed (in #33 already)

2) The system_list_reset() is performed in dfac(), module_enable(), and module_disable() already. _system_update_bootstrap_status() invoked it another time, needlessly.

3) Incorporated the system_cache_flush() suggestion. Thanks!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new interdiff.txt2.07 KB
new drupal8.flush-rebuild.36.patch22.19 KB

Minor phpDoc adjustments.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs review » Reviewed & tested by the community

ok, i think this is ready to fly. setting to RTBC so catch will see it.

chx’s picture

chx commented
Status: Reviewed & tested by the community » Needs work

Huh. I have a lot of problems with the new doxygen. The old had examples which seem to be gone. Also,

+ * Flushes and resets all caches, and rebuilds data structures.

I presume you mean All static caches are reset and all cache bins are flushed. And then what is rebuilt...? That's where it gets confusing. What about adding to

+ * - Rebuilds and synchronizes data structures (invoking hook_rebuild()).

"The following core modules implement this hook: block doing bananas" etc. The big problem is that without an intimate knowledge of core you have no chance to know whether something is rebuilt on this call or the next request when a cache miss occurs and is stored into cache.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt3.88 KB
new drupal8.flush-rebuild.39.patch23.4 KB

#38 asks for phpDoc clarification on what subsystem caches are cleared and what subsystem data is rebuilt. Also, to clarify in phpDoc that, in fact, the menu router has not been rebuilt yet when hook_rebuild() is invoked.

  1. Moved the actions_synchronize() from dfac() into its proper home: system_rebuild(). There was no reason to have that in dfac() in the first place.
  2. Clarified hook_rebuild() phpDoc regarding the final menu router rebuild.
  3. Clarified dfac() phpDoc.
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Applied the patch, executed update.php, then my theme information was lost on the final confirmation page.

@aspilicious: Nope, menu_rebuild() is not a hook implementation of hook_rebuild().

Ah-ha. Then how do you explain this?:

var_dump(module_implements('rebuild'));
array(5) { [0] => string(5) "block" [1] => string(5) "field" [2] => string(4) "menu" [3] => string(4) "node" [4] => string(6) "system" }

We do have a menu.module....

Anonymous’s picture

Anonymous (not verified) commented

Hahahaha. I mean, stabbity stab stab.

Anonymous’s picture

Anonymous (not verified) commented
Issue summary: View changes

Updated issue summary.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt14.19 KB
new drupal8.flush-rebuild.42.patch37.57 KB
  1. Renamed menu_rebuild() into menu_router_rebuild().

    Note: Doing this revealed many calls to menu_rebuild() throughout the system that look very dubious to me. I'll create a follow-up issue to analyze whether those invocations are actually correct and needed, or whether we're unnecessarily rebuilding the router repeatedly. In any case, separate issue.

  2. Moved the re-initialization of the maintenance theme into dfac() itself, so install.php and update.php and any other special script should remain to work as before.

Also updated the issue summary with API changes, as well as some more notes about other/related efforts elsewhere.

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.42.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.43.patch37.6 KB 
+++ b/core/includes/common.inc
@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+  menu_rebuild();

D'OH! Forgot the instance in dfac() ;) No other change, so the last interdiff still applies.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

The bastard is green! Good work! ;)

+++ b/core/includes/common.inc
@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ * This means the entire system is reset to be effectively empty. After that is

The entire system isn't empty, the content stays. This should be phrased better.

+++ b/core/includes/common.inc
@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+ * @todo This function resets all static caches. The theme is not initialized
+ *   anymore and all previously added JavaScript and CSS is gone. But since that
+ *   is the whole point of this function, all callers need to take care for
+ *   backing up needed variables and properly re-initializing the theme on their
+ *   own. See install_finished() for an example. Normally, this function is

Outdated comment? It's now set to the maintenance theme and install_finished() doesn't re-initialize anything.

+++ b/core/includes/common.inc
@@ -7289,50 +7289,104 @@ function drupal_implode_tags($tags) {
+  // Rebuild the schema and cache a fully-built schema based on new module data.
+  // This is necessary for any invocation of index.php, because setting cache
+  // table entries requires schema information and that occurs during bootstrap
+  // before any modules are loaded, so if there is no cached schema,
+  // drupal_get_schema() will try to generate one, but with no loaded modules,
+  // it will return nothing.
+  drupal_get_schema(NULL, TRUE);

ouch! Imho that should be fixed in the bootstrap.. Well, I guess that's a no trivial task and is best done by the bootstrap cleanup anyway. Maybe we should add a @todo here to remove it once the bootstrap has been cleaned up?

+++ b/core/modules/system/system.api.php
@@ -2087,22 +2087,63 @@ function hook_mail($key, &$message, $params) {
+ * This hook is only invoked after the system has been completely cleared;
+ * i.e., all previously cached data is known to be gone and every API in the
+ * system is known to return current information, so your module can safely rely
+ * on all available data to rebuild its own.

hm, not necessarily. There might be data that needs to be rebuilt in order to be available to the module. E.g. consider a block_node module "building" blocks for each node type using a rebuild mechanism - it won't see new node types as hook_node_rebuild() is invoked later on by default. Thus, we should clarify that the order of rebuilds might be important (which can be altered as usual for hooks).

That said, modules rebuilding data should clear their caches *again* once the have rebuilt the data. Taken the node-block example, the block rebuild might have primed the node type cache which might contain old information once node types got rebuilt. Thus, node_types_rebuild() should make sure to update/clear the node-type cache once rebuilt.
Also, that shows that rebuilding data is more painful than the regular cache-approach of building the data on demand, as possible dependencies have to be resolved manually. Maybe we should try to clarify on when it is a good idea to pursue which approach (cache vs rebuild)?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.46.patch37.75 KB

1) Fixed docs on "empty."

2) Fixed docs on the theme reset. (removed the todo)

3) The schema reset and corresponding comment is not touched by this patch, merely relocated. Separate issue.

4) Honestly, I'd like to see a separate issue for the multidimensional rebuild/cache issue you're describing. Because I don't really get what you're trying to explain. ;) That issue will be able to rely on hook_rebuild() being there, so can focus on that operation only, and can properly discuss that detail. Discussing it in here only leads to more confusion around this patch. I'd really like to get rid of this issue here first, because the change is anything but trivial.

EDIT: Last patch had "0 passes". Let's watch out.

chx’s picture

chx commented
StatusFileSize
new 1404198_47.patch37.92 KB
new 1404198_47_interdiff.txt753 bytes

I added further doxygen. Yes, we usually don't list the core implementation of a hook but this is so hard more documentation can not hurt.

xjm’s picture

xjm
she/her
Primary language English
commented

Still "0 passes."

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 1404198-49.patch37.43 KB

updating patch for latest commits, reroll only except for a menu_rebuild() call in install.core.inc which was removed in latest head.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 1404198-50.patch37.83 KB

hmm, there was a menu_rebuild() in standard_install() that wasn't ported over.

Status: Needs review » Needs work
Issue tags: -Framework Initiative, -API change, -API clean-up

The last submitted patch, 1404198-50.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review

#50: 1404198-50.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Framework Initiative, +API change, +API clean-up

The last submitted patch, 1404198-50.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) commented

wow. so, inside the deep rabbit hole i just dug around in, i discovered something rather obvious.

in the parent drupal, when you properly blow away all caches and rebuild node types, you don't get any new node types from modules enabled in the child drupal. because module_implements(), $deity bless it, checks to see if you have loaded the module's implementation before returning it.

so, adding the three lines at the bottom to testCommentEnable() makes the test pass:

   // Enable core content type modules (book, and poll).
    $edit = array();
    $edit['modules[Core][book][enable]'] = 'book';
    $edit['modules[Core][poll][enable]'] = 'poll';
    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
    // YEAH! Load that code!
    drupal_load('module', 'poll');
    drupal_load('module', 'book');

but is also clearly completely wrong.

Anonymous’s picture

Anonymous (not verified) commented
Status: Needs work » Needs review

here's a patch that makes DrupalWebTestCase::resetAll() load all code for enabled modules before calling drupal_flush_all_caches() to make sure we don't rebuild stuff in the parent Drupal and miss any implementations.

so the only change #50 is to resetAll():

 protected function resetAll() {
    // Make sure we load the code for all enabled modules. Module's enabled via
    // form posts from the parent Drupal will not have their code loaded. This
    // can lead to module hooks not being returned from module_implements(),
    // which will check that hook implementations are in memory.
    foreach (module_list(TRUE) as $module) {
      drupal_load('module', $module);
    }

    // Clear all database and static caches and rebuild data structures.
    drupal_flush_all_caches();

    // Reload global $conf array and permissions.
    $this->refreshVariables();
    $this->checkPermissions(array(), TRUE);
  }
Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new 1404198-55-ffs.patch39.45 KB

umm. with patch this time.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new interdiff.txt1.51 KB
new drupal8.flush-rebuild.57.patch39.3 KB

Thanks for debugging this, @beejeebus!

So this patch should actually prevent that from happening in the first place. dfac() is supposed to be able to fully re-initialize and build or rebuild Drupal into a state in which it can fully operate. Regardless of whether there currently is bogus or outdated information or not.

The actual cause and problem is the static variable in module_list().

Apparently, module_list() calls into system_list(), which uses a drupal_static() already, so the system list cache is fully flushed and reset by dfac(), but module_list() is not.

That can obviously call for trouble only. So I'd suggest to fix that static instead.

Interdiff is against #50.

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.57.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt1001 bytes
new drupal8.flush-rebuild.59.patch40.16 KB

Thankfully, #1541958: Split setUp() into specific sub-methods made the operations being performed in setUp() much more clear. To explain the fatal errors and quote from there:

it's totally inane to changeDatabasePrefix() before prepareEnvironment(), 'cos the latter attempts to backup tons of stuff by querying the parent site. The fact that this worked was solely caused by a weird mix of environment information and lots of cached variables.

Status: Needs review » Needs work

The last submitted patch, drupal8.flush-rebuild.59.patch, failed testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt503 bytes
new drupal8.flush-rebuild.61.patch40.25 KB

So #59 resolved a lot of failures. Now dfac() only needs to ensure that all modules that are supposed to be enabled are actually loaded. Added in this patch.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Victory! :)

That said, if we don't want to introduce the new drupal statics, then we could as well reset them manually within dfac(). However, I think that adding them is the proper change. Especially because of the module_list() vs. system_list() difference.

Anonymous’s picture

Anonymous (not verified) commented

i want to set this to RTBC, but i guess fago should have a look first?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

If #1541958: Split setUp() into specific sub-methods lands first, this patch will need to be re-rolled. To do so, just skip/remove the hunk in drupal_web_test_case.php.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Needs work

4) Honestly, I'd like to see a separate issue for the multidimensional rebuild/cache issue you're describing. Because I don't really get what you're trying to explain. ;) That issue will be able to rely on hook_rebuild() being there, so can focus on that operation only, and can properly discuss that detail. Discussing it in here only leads to more confusion around this patch. I'd really like to get rid of this issue here first, because the change is anything but trivial.

True. Ok, let's do so. 

+++ b/core/includes/module.inc
@@ -444,10 +461,10 @@ function module_enable($module_list, $enable_dependencies = TRUE) {
+      _system_update_bootstrap_status();
       system_list_reset();
       module_list(TRUE);
       module_implements_reset();
-      _system_update_bootstrap_status();

Why do we re-order that in module_enable()/disable()?

That looks problematic, as updating the bootstrap status relies upon module_implements() which relies upon module_list(), thus both caches need to flushed before we call this function. Otherwise, the new module might be missed my _system_update_bootstrap_status().
That said, clearing all that different module-related function and their interdependencies is quite a mess too. This would deserve its own clean-up issue.

Else, patch looks good to me.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs work » Needs review
StatusFileSize
new drupal8.flush-rebuild.66.patch38.86 KB

The concern in #65 sounds valid to me. Thus, I've reverted all changes to _system_update_bootstrap_status() in attached patch.

fago’s picture

fago
Primary language German
Location Vienna
commented
Status: Needs review » Reviewed & tested by the community

Good!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Hm. I'd prefer the change for setUp() to land with the follow-up patch in #1541958: Split setUp() into specific sub-methods instead of this one, because the follow-up fix really belongs over into that issue/patch.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Big clean-up. Thanks for all the careful reviews.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Separate database cache clearing from static cache clearing and data structure rebuilding » Change Notice: Separate database cache clearing from static cache clearing and data structure rebuilding
Priority: Major » Critical
Status: Fixed » Active
Issue tags: +Needs change record

Thanks. This is a huge API change and obviously needs a change notice. :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Title: Change Notice: Separate database cache clearing from static cache clearing and data structure rebuilding » Separate database cache clearing from static cache clearing and data structure rebuilding
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

Created two change notices:

http://drupal.org/node/1561490
http://drupal.org/node/1561492

moshe weitzman’s picture

moshe weitzman commented

Nice patch, but the name menu_router_rebuild() is misleading. We are rebuilding two subsystems here - menu router and menu links. Conflating the two is a step backward IMO. Hopefully we can fix that in a followup.

xjm’s picture

xjm
she/her
Primary language English
commented

Awesome!

Is there a followup issue for #72? If so, can we link it here, and if not, should we file one?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Hm. I'm not sure whether we need that follow-up - the Web services initiative will rewrite the router system anyway. It's possible that these changes will remove the double notion.

Automatically closed -- issue fixed for 2 weeks with no activity.

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Updated issue summary.