Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem
- Pre-compiled + dumped data is read in again instead of being rebuilt in
drupal_flush_all_caches()
["dfac()"] — the previous container is still in memory. - Drupal is not able to boot anything at all, if available vs. registered vs. requested services differ.
Details
dfac()
is required when the system configuration is changed in significant ways and all caches need to be flushed and base data of subsystems and services needs to be rebuilt.dfac()
may also be able to successfully rebuild the system when definitions of "non-essential" services are changed (in code). "Non-essential" services means all services that are not in the critical path.- However,
dfac()
fails in case the definition of a service has changed and the service is requested before the kernel could be rebuilt. dfac()
always fails for base system services in the critical path; it requires the config system, lock system, variable system, and module system to operate.- Also,
dfac()
should not be invoked prior to DRUPAL_BOOTSTRAP_CODE, since it needs to call into all enabled modules to properly flush all caches and rebuild data. - A low-level rebuild is required when essential base system services are defined/registered differently, extended, or possibly changed altogether.
Proposed solution
- Fix
dfac()
to properly rebuild the kernel in cases it is possible. - Introduce
drupal_rebuild()
for deadlock cases in which the current kernel and caches are corrupt and need to be rebuilt.
Notes
- Invoking
/unfreeze.php
on an outdated D8 site allows to get back to a working, pre-existing D8 dev site without having to re-install. - This patch only educates how to use
drupal_rebuild()
in a suggested/unfreeze.php
file, but does not introduce the file itself. Introducing a rebuild script is left for a separate issue. - Manually deleting the compiled container files only via
sudo rm -rf sites/default/files/php
is a workaround that might resolve the deadlock situation in certain situations.
Todo
- Tests? Write a broken container to disk and try to recover from it?
Comment | File | Size | Author |
---|---|---|---|
#89 | 1872522_84.patch | 5.5 KB | chx |
#89 | interdiff.txt | 1.82 KB | chx |
#84 | 1872522_84.patch | 7.53 KB | YesCT |
#84 | interdiff-83-84.txt | 1.4 KB | YesCT |
#83 | 1872522_83.patch | 7.52 KB | desmondmorris |
Comments
Comment #1
xjmSending patches to the bot.
Comment #2
xjmNot the block module.
Comment #5
sunThe fix and approach taken in my existing patches is correct, however:
dfac() is actually not able to handle a deadlock situation of a corrupted kernel. Attached patch introduces a new wrapper function, and elaborately explains why it is required and what is being done.
I'll rewrite the issue summary in a moment.
Comment #5.0
sunUpdated issue summary.
Comment #6
chx CreditAttribution: chx commented+ // @todo Once file system settings are converted to configuration, this means
+ // a massive circular dependency, since the configuration system requires a
+ // working kernel.
BootstrapConfigStorageFactory::get
Comment #8
sunre: #6:
BootstrapConfigStorageFactory
is not of any help, unlessPhpStorageFactory
gets the configuration service injected.That's left for another issue to figure out though.
Comment #9
sunComment #11
effulgentsia CreditAttribution: effulgentsia commented1. The comment says "synthetic services", but what's actually returned also includes non-synthetic services for which get() has already been called.
2. 'service_container' is a synthetic service that we do not want to persist.
This fixes the second issue only (and not very nicely). CNR for bot.
Comment #12
effulgentsia CreditAttribution: effulgentsia commentedHere's a more robust fix to both parts of #11.
Comment #14
effulgentsia CreditAttribution: effulgentsia commentedThe test failure in #12 appears to be a latent bug with how/when LocaleConfigSubscriber gets added to the event dispatcher. The bug appears to get surfaced by the changes to dfac(), but masked by HEAD's overly aggressive service persistence across container rebuilds. To untangle it, I suggest committing #1187726-135: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) first.
Comment #14.0
effulgentsia CreditAttribution: effulgentsia commentedUpdated issue summary.
Comment #15
sunWhen rebuilding an already compiled + possibly dumped container, then we're acting on a
Container
- not aContainerBuilder
.Therefore, unless I'm missing something big, there is no
getDefinitions()
method — all of the build information is stripped from a compiled container.Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThat line of code is only called within buildContainer() where we know we have a ContainerBuilder. Looks like #1187726-138: Add caching for configuration / rework config object loading (Was: Memory usage and i/o from config objects) is RTBC, so once that's committed, I'll reroll #12 and try to work through the Locale test failure.
Comment #17
effulgentsia CreditAttribution: effulgentsia commentedAnother spin-off for addressing the test failure: #1878512: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling
Comment #18
sunRe-rolled against HEAD.
Comment #20
sunIs anyone able to explain the failures?
I need this rebuild almost every day...
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedComment #23
sun#21: unfreeze.21.patch queued for re-testing.
Comment #25
effulgentsia CreditAttribution: effulgentsia commentedThis combines #21 with #1878512-12: Clean up Simpletest's kernel/container preparation/rebuild logic now that the kernel is responsive to module enabling. While the issues have differing scopes, they also have some overlap, and the individual patches are small enough, that even combining them into this one patch is still reviewable, I think.
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedAnd a small comment tweak.
Comment #28
effulgentsia CreditAttribution: effulgentsia commented#26: unfreeze.26.patch queued for re-testing.
Comment #29
tim.plunkett#26: unfreeze.26.patch queued for re-testing.
Comment #31
aspilicious CreditAttribution: aspilicious commentedReroll.
1) Not sur if the reroll is 100% correct as some stuff changed
2) I don't need any credits for this one.
Comment #33
aspilicious CreditAttribution: aspilicious commented#31: unfreeze.31.patch queued for re-testing.
Comment #34
aspilicious CreditAttribution: aspilicious commentedtestbot fail reference: FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed attempting to get list of tests from run-tests.sh.
Comment #36
aspilicious CreditAttribution: aspilicious commentedNew try
Comment #38
aspilicious CreditAttribution: aspilicious commentedwrong patch...
Comment #40
BerdirAs it looks now, this is not going to happen but the public path will move to $settings. So not sure i that needs to be documented.
Comment #41
BerdirBasic re-roll, renamed some rebuild/prepareContainer() method calls. Most of the test fails seem to be about theme() thinking that the module system is not yet fully bootstrapped?
Comment #43
BerdirHere's a reroll of #21. I think after the module handler went in, the test related changes became more complicated, what about getting this in separately?
Comment #45
chx CreditAttribution: chx commentedThis seems stalled, taking it for sprint work.
Comment #46
chx CreditAttribution: chx commentedThis is about half-rewrite. I have added a new rebuild.php that can be requested via PHP and a rebuild.sh that calculates the token necessary. It's a small one but I think it'll do the job. It might be the answer to some drush woes as well. I am not terribly sure how helpful the interdiff is...
Comment #48
chx CreditAttribution: chx commentedComment #50
chx CreditAttribution: chx commentedThis is solvable but the solution makes me cringe. It also makes me question wtf are we doing in tests re container.We have
$this->container
in tests. If this is not the same object asDrupal::getContainer()
then whatever reset, set, etc etc we do via the container on one is not going to affect the services on the other. That's just bad. So in order to keep these together,WebTestBase
already does two full container rebuild mostly because of this. After this patch, every time you do adrupal_flush_all_caches
somewhere, your test needs to redo the$this->container = \Drupal::getContainer()
part. I added that toresetAll
hoping that will be enough but I can't be sure.I see two exits:
Oh and both is an option too :D
Comment #52
xmacinfoThe list can change when :
Comment #53
chx CreditAttribution: chx commentedThe "list of modules" in question is the list of enabled modules and that can only change by deletion but, given mostly the patch passes, this is moot now. I will look into the failing test.
Comment #54
chx CreditAttribution: chx commentedI filed separate issues for the interdiff #1948650: Update is broken and #1948702: module_hook is broken for disabled modules and included hooks . Once those two are in, just press re-test on #50. This patch here just shows that once those two are in, we are in business.
Comment #55
jibranThis should be postponed as per #54
Comment #56
chx CreditAttribution: chx commentedNope, nor those patches nor #54 merits a postpone yet -- we need reviews first and then we agree it's ready, we can postpone it then waiting for those.
Comment #57
jibran#50: 1872522_50.patch queued for re-testing.
Comment #58
jibranHere is a reroll of #50 run into conflict
and kept the head code as per @chx recommendation on IRC
Comment #59
moshe weitzman CreditAttribution: moshe weitzman commentedThe approach and the code look good and useful to me.
Comment #60
YesCT CreditAttribution: YesCT commented#58: 1872522_58.patch queued for re-testing.
Comment #61
ParisLiakos CreditAttribution: ParisLiakos commentedLets update this comment if we are keeping updateModules()
Lets store somewhere module handler instead of calling Drupal::service('module_handler') all the time
where is this needed?
Comment #62
tstoecklerAlso we can now use the new and shiny \Drupal::moduleHandler() directly.
Comment #63
YesCT CreditAttribution: YesCT commentedchanging status to needs work to address #61 and #62
Comment #64
chx CreditAttribution: chx commentedComment #66
moshe weitzman CreditAttribution: moshe weitzman commented#64: 1872522_64.patch queued for re-testing.
Comment #68
chx CreditAttribution: chx commented*waves hand* this is not the module handler you are looking for.
Comment #69
chx CreditAttribution: chx commentedComment #70
dawehnerCan we have a @file to describe what this is all about?
This is an intersting and helpful idea. Can we please make it translatable.
Comment #72
chx CreditAttribution: chx commentedAdded some documentation. Translateable is hard because you are practically guaranteed that Drupal is not working and both reading the current language and the translation in that apgnauge is a significant challenge. I think we might be able to do something better in a followup.
Comment #73
dawehnerHe, you broke index.php :)
Thanks for the documentation. please add a @file
Comment #75
chx CreditAttribution: chx commentedmeh.
Comment #76
aspilicious CreditAttribution: aspilicious commentedWho is going to create the "rebuild" page?
Comment #77
chx CreditAttribution: chx commentedIf you think of http://drupal.org/documentation/rebuild I will write it.
Comment #78
desmondmorris CreditAttribution: desmondmorris commented#75: 1872522_75.patch queued for re-testing.
Comment #80
desmondmorris CreditAttribution: desmondmorris commented@chx looks like that patch fails with the latest core build. here is my attempt at the updated patch.
Comment #81
desmondmorris CreditAttribution: desmondmorris commentedLast patch did not include file additions. Lets try this again
Comment #82
jibranThis change is not required see #1982984: Create Drupal::entityManager for improved DX.
Comment #83
desmondmorris CreditAttribution: desmondmorris commentedThanks @jibran. Updated per #83 - #1982984: Create Drupal::entityManager for improved DX
Comment #84
YesCT CreditAttribution: YesCT commentedI was reading and noticed a mis-spelling.
Fixed comment line wrap and moved a comment while I was at it.
Comment #85
jthorson CreditAttribution: jthorson commented#83: 1872522_83.patch queued for re-testing.
Comment #86
tim.plunkettI'm really glad this docblock is here. Very helpful.
The only possible change I'd say we would want to make here is moving the try/catch inside drupal_handle_request(), but there are pros and cons to both.
---
Overall, the changes to existing files make 100% sense to me.
I'm less certain about core/rebuild.php and core/scripts/rebuild_token_calculator.sh.
Not enough not to commit, just that I'd like catch's signoff here.
Comment #87
catchI'm also not sure about core/rebuild.php and core/scripts/rebuild_token_calculator.sh. We've got existing issues to add something like that (although those were originally due to the D7 registry) and I'm sure there's discussion on those that isn't reflected here. For example why a cli token generator and not a flag in $settings similar to update_free_access? The latter would allow people with no cli access to rebuild for example, whereas this makes things command line only - but if it's command line only then why not directly rebuild in the shell script anyway?
I'd really like to get the bug fix in though (and that bit looks great), so could we maybe do a new issue?
Comment #88
catchComment #89
chx CreditAttribution: chx commentedSame patch sans rebuild files.
Comment #90
alexpottCommitted 750b527 and pushed to 8.x. Thanks!
Comment #91
neclimdulMight I suggest 1) we write the node and 2) make the url a link?
Comment #92
xmacinfo@neclimdul: Looks like you need to open a new issue. I do not think your suggestion applies here.
Comment #93
genjohnson CreditAttribution: genjohnson commentedI've moved neclimdul's suggestion to create the node to #2003204: Create http://drupal.org/documentation/rebuild.
Comment #94
genjohnson CreditAttribution: genjohnson commentedneclimdul's suggestion to make the URL a link has been moved to #2003224: Make http://drupal.org/documentation/rebuild a link in index.php.
Comment #95
genjohnson CreditAttribution: genjohnson commented@chx: The page http://drupal.org/documentation/rebuild is ready for you to add content.
Comment #97
webchickThe error message that was added to index.php here is a bit vague. I filed #2056915: Improve error message when drupal_handle_request() fails so we can try and improve it.
Comment #97.0
webchickChange author to sun to match author of previous issue & summary. --xjm
Comment #98
sunThanks for moving forward here.
However, there are two major issues with the committed change:
Catching any kind of Exception in index.php harms DX and cannot be desired.
That was never the intention of this issue. In fact, as a developer, I was happy, and I do want to see where exactly the system blows up.
In fact, the catch-all for any kind of exception is misleading in ~99% of all cases. Just because an exception was thrown doesn't inherently mean that the DI container has to be rebuilt.
In short, this issue never had the intention to change the regular runtime behavior of Drupal.
I am very confident that we want to revert that try/catch change to index.php.
The actual problem in the OP was not resolved.
It's good to see the fixes for
drupal_flush_all_caches()
& Co in core now, but none of these code paths can be reached in the first place; the system blows up much earlier already.Earlier patches added a new
drupal_rebuild()
function to core, without getting into the can of worms of arebuild.php
or any other interface.The most important goal was to get the fundamental facility into core. Whether you invoke that facility with a custom script, via Drush, or through any other mechanism doesn't matter.
What matters is that there is a defined (API) way to resurrect your site when encountering the situation.
I'm not sure how to proceed here, as it appears that others are satisfied with the solution, but as the original reporter/author of this issue, I'm happy about the progress, but not satisfied with the solution, as it doesn't resolve the actual problem and introduced new problems.
Comment #99
olli CreditAttribution: olli commentedHere is the issue for rebuild.php.
#2097189: Add a rebuild script