Here's a patch that adds a failing test to the bundles branch. The test passes if you change BundleTest::setup() to enable the 'bundle_test_dependent' module, but the point of this test is to not do that. I think module_enable() needs to cause an immediate rebuild of the container and make the kernel use it. An example for why this is needed is suppose you use the admin/modules page to enable a module that depends on another module (e.g., forum on taxonomy). The hook_enable() implementation of the module that has the dependency (forum) should be able to use the dependent module's (taxonomy) services.
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedApologies. I got my terminology wrong. bundle_test (or at least this callback) depends on bundle_test_dependent, which is therefore incorrectly named and should be bundle_test_dependency. i.e., if A depends on B, then B is the dependency and A is the dependent (I think, correct me if I'm wrong). Sorry if this caused any confusion.
Comment #2
katbailey CreditAttribution: katbailey commentedI have a feeling this might be in principle impossible. Rebuilding the container mid-request has to mean instantiating a brand new container, right? But the very HttpKernel instance that is handling the request is managing the scope of the old container in doing so. Thinking of making it use the new container makes me think of an Escherian hand drawing itself. If anyone can see a way around this that I'm not seeing, I'd love to hear it.
Comment #3
Crell CreditAttribution: Crell commentedThe only way to make this work, I think, is to Enable the module, regenerate the DIC, and then do a self-redirect to run the install/update hooks now that the DIC is updated. Which of course only works in HTTP mode, not CLI mode, although I think there's some trick to "reboot" a CLI, too.
Is there a specific use case that cannot be otherwise solved, rather than a "nice to have" that we get now by virtue of playing fast and loose with system state (which is something we're trying to get away from)?
Comment #4
lsmith77 CreditAttribution: lsmith77 commentedi think its not wise to execute code synchronously by a process that is installing said code. it will always be very hard to ensure a defined behavior in this case since its quite common for boot strap processes to make the assumption that all pieces are available. breaking this assumption will always add a lot of complications to very core logic and i dont really see what use case requires the behavior illustrated by the failing test ..
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedAt this time, because #1599108: Allow modules to register services and subscriber services (events) doesn't add any module-specific DIC entries for any core modules, I don't think this issue needs to block that one from going in, but once that issue does go in, I think that some retitled version of this issue (e.g., "Integrate system_modules_submit() with DrupalKernel") will need to be a critical; otherwise once modules start adding module-specific DIC entries and using them within their API functions, those functions will fatal error when called from a dependent module's hook_enable().
That will depend on what modules end up putting into their bundle classes. As an example, forum_enable() calls taxonomy_vocabulary_save() and node_types_rebuild()/node_type_get_types(). Will taxonomy_vocabulary_save() require DIC services from the taxonomy bundle, or will node_type_get_types() require DIC services from the node bundle? I don't know what our first actual use case will be, but I'm sure we'll hit many.
If we decide that the solution we want for web-based triggers of module_enable() is to issue a self-redirect, then we'll need a separate issue for figuring out CLI. I think it makes sense to figure out the web-based issue first, because I'm assuming CLI scripts, if nothing else, can reinstantiate a new DrupalKernel when they need to. Despite lsmith77's warning in #4, I also don't want to prematurely rule out the possibility of adding a rebuildContainer() method to DrupalKernel, and preserving the synchronous module_enable() we have now. I'm not saying we should do that, but I see it as a legitimate option to have on the table if we run into too many difficulties with a self-redirect approach.
Comment #6
pounardAgree with #4, maybe it's time for Drupal to rethink the way it handles modules. Modules probably should be installed/uninstalled without the need of being enabled, this cause various problems, at site install time where the site needs to be bootstrapped unfinished and when enabling multiple modules at once on a live site, etc.. due to all stuff that cores has to do, such as clearing all the caches, calling all possible hooks, enabling modules, rebuild the runtime environment at runtime...
In actual core, when you enable a module, it's hook_init() and hook_boot() has not been run, and so in bundles services are not registered, which seems totally valid to me. Modules should not need their own services when being installed, considering their aren't even configured yet.
Comment #7
catchIn that case we'd need to remove hook_install(), hook_schema(), hook_enable() and any other hook that's invoked at install time. On the other hand I don't think anyone loves the module install process as it is at the moment so rethinking it a bit sounds good.
Comment #8
Dries CreditAttribution: Dries commentedI committed #1599108: Allow modules to register services and subscriber services (events) after having given it a lot of thought and discussion, but decided that we need to escalate this to 'critical' as a result of that. I've also moved it into the core issue queue. Please don't demote the priority of this issue. Let's keep working on a good solution. Thanks for all your help! :-)
Comment #9
tim.plunkettI'm reasonably sure this is causing all the failures I'm seeing in Views when we try to use bundles.
Rerolled after #1727538: WebTest tests should have access to the correct DIC.
Comment #11
moshe weitzman CreditAttribution: moshe weitzman commentedJust to be clear, Tim has rerolled the test that proves that we have a critical bug. We are not any closer to solving it.
I'm curious what bundle services are added by Views and needed at module_enable() time.
Comment #12
tim.plunkettCurrently, most Views provide either a page callback or a block. hook_menu and hook_block_info are triggered at that time, and need to load Views.
See #1727266: Lazy load plugin managers with dependency injection for the code we're attempting.
Comment #13
sunSo I guess the essence of #12 is that drupal_flush_all_caches() is called in the same request in which the new module is enabled/disabled.
I also guess we only have two options:
1) Rebuild the service container within the same request, before invoking hook_enable() + friends, and recompiling it at the end of the request (or in the next).
2) Try to set a flag/marker or something to signify the enabling/disabling of a module for the next request and deleting the currently compiled container before the request ends, and lastly, also try to handle race conditions on large-scale/high-traffic sites appropriately (which sounds close to impossible to me).
Comment #14
chx CreditAttribution: chx commentedA twin (might need to be enjoined, even) #1759582: Figure out when to delete compiled DIC files
Comment #15
andypostAnother use case is ban module to hook into request object #1161486-25: Move IP blocking into its own module
Comment #16
chx CreditAttribution: chx commentedalippai wrote this http://privatepaste.com/9eec5e910b module, one wonders whether we could use the idea here.Edit: this is essentially the same as 2.) from #13 , implemented in a module. As for the thundering herd problem, can't we use a lock / semaphore / something?
Comment #17
tim.plunkett#1763974: Convert entity type info into plugins is also blocked on this.
Comment #18
tim.plunkettClarifying the title as I understand it. It's not just about bundles provided by the module being enabled, it's all bundles.
Comment #19
tim.plunkett@katbailey asked for some more info on how we were using this, and how it was breaking.
Views provides many plugin types, currently all controlled by the ViewsPluginManager.
In order to get a plugin instance, we had a procedural function that looked like this:
Now, as a deprecated wrapper (we have yet to phase it out), we have:
However, as we add more test coverage, we're seeing fails like this:
Here's the relevant call stack for one such occurence: https://privatepaste.com/ddf8081988
Note lines 13 and 14. l() is being called, which is calling theme_get_registry(), and then everything goes to hell.
So, I tried putting this into ViewTestBase:
And it all worked out.
Other issues, like #1763974: Convert entity type info into plugins, attempt to replace parts of hook_entity_info() with plugins. But entity_get_info() is called very early as well, and breaks similarly: https://privatepaste.com/f81a11a981
Comment #20
tim.plunkettSee also, #1786990: [Module]Bundle is registered too late in WebTestBase::setUp(), #1801082: drupal_flush_all_caches() assumes a web request - "The service definition "router.builder" does not exist. "
Comment #21
effulgentsia CreditAttribution: effulgentsia commentedI don't think so. I think this issue is specifically about new modules enabled in module_enable() that then have DIC services consumed in the same request (e.g., #5), so reverting issue title to pre #18. #19 contains good info for #1786990: [Module]Bundle is registered too late in WebTestBase::setUp(), which is probably the more pressing problem to resolve for Views work in progress.
Comment #22
tim.plunkettI agree with the title revert. Between this and the two issues I linked in #20, those are all of the problems we've seen.
Comment #23
tim.plunkett#1786990: [Module]Bundle is registered too late in WebTestBase::setUp() was committed, which fixes half of VDC's problems with this.
Here is the current failure:
Comment #24
xjmClarifying #23, this straight up blocks #1805996: [META] Views in Drupal Core. Whenever you enable Views--not only in tests but in the actual site environment--you get the error above.
Comment #25
chx CreditAttribution: chx commentedBlocked Views is bad/
Comment #26
xjmRelated: #1331486: Move module_invoke_*() and friends to an Extensions class
Comment #27
xjmOopsie.
Comment #28
katbailey CreditAttribution: katbailey commentedAlso related: #1759582: Figure out when to delete compiled DIC files, which is a follow-up for #1706064: Compile the Injection Container to disk, which is hopefully going to be RTBC'd shortly :-)
I'm planning to reroll #1331486: Move module_invoke_*() and friends to an Extensions class today. That's not a blocker for this issue, it's just that if we're rethinking the whole module_enable process, it probably makes more sense to do so with the work in that patch in mind.
Comment #29
chx CreditAttribution: chx commentedThis can't be solved without #1706064: Compile the Injection Container to disk but it's blatantly trivial with it so moving there.
Comment #30
chx CreditAttribution: chx commentedOk let's not merge this yet. It needs to be postponed on that one, however. http://drupal.org/files/interdiff_1320.txt is the patch that will solve this issue, likely.
Comment #31
ybabel CreditAttribution: ybabel commentedwscci-bundles-module_enable.patch queued for re-testing.
oops !
sorry, I was trying to figure out how to make VDC works.
I get that error when I try to install the sandbox :
Comment #32
ybabel CreditAttribution: ybabel commentedAppliying http://drupal.org/files/interdiff_1320.txt work for me (with the last VDC sandbox)
even if it gives me that errors:
but now the views UI are accessible !!! I can use Views, which was impossible before this patch.
my frontpage is back also :-)
Comment #33
xjmThanks for testing @ybabel! And yep, this is the issue currently blocking the sandbox from working smoothly.
Re-postponing on #1706064: Compile the Injection Container to disk. Once that is in, this issue can be fixed, and then we can continue work on the sandbox issues. You can help us discuss anything you encounter in the sandbox queue. :)
Comment #34
xjm#9: drupal-1708692-9.patch queued for re-testing.
Comment #36
xjmRebased #9 and tweaked the docs.
Comment #38
fubhy CreditAttribution: fubhy commentedMerged the test and a slight modification of @chx's solution from #30 and simplified the test a bit to just check for the existence of the service id. Also added a test for module disable (which also fails). Tried to rebuild the container as well but for some reason it did still not pass. Suggestions welcome :)
PS: I am not 100% sure if it is okay to go with $modules_installed instead of doing a fresh system_list/module_list call. It should be the same actually.
Comment #40
chx CreditAttribution: chx commentedComment #41
xjmTrying something.
Comment #42
xjmOkay I am way out of my depth so I didn't get very far. But here's one fail fewer!
I tried to also do the rebuild in
module_disable()
but couldn't get that assertion to pass, so left that as a@todo
for now.Comment #44
BerdirWe need to pass in all modules, not just the one that we just enabled. This patch is based on the approach of chx's interdiff in #30 and adopted to the way kernel compilation works now.
Comment #45
effulgentsia CreditAttribution: effulgentsia commented- Simplified the test to reuse the bundle_test module instead of adding a bundle_test_dependent module.
- Changed so that module_(en|dis)able() doesn't need to boot a new kernel (hard-coding constructor arguments that might not match the initial environment), but can reuse the same kernel.
The main difficulty in this issue though isn't the implementation: it's that several of the early comments in this thread expressed opposition to making container changes mid-request at all. But per #7, that would require a complete change to our module_enable() process to not call hooks or rebuild the theme registry in the same request, but instead immediately redirect and do those things in the following request. Until we fix this issue either with that or with something like this patch, we can't add Views to the Standard install profile. So, how should we proceed?
Comment #46
BerdirOh, yes, that looks much nicer ;) Wasn't aware that the kernel is in the container as well aka forgot that.
Yes, there's an issue to make an install-time kernel/router and and clean up our current mess.
Comment #47
BerdirAlso, I understand the problems outlined in those initial comments (I think) but changing module_install() to require a site redirect sounds like a huge change which would significantly change the way many things in Drupal work (like tests, for example). For me personally, that doesn't sound feasible at this point in time.
Comment #48
sunThis looks great to me.
Fixing the apparent problem will not disallow us from rethinking in-process/intra-request extension management. However, I do agree with previous posters that it appears rather unlikely for us to be able to completely revamp this logic for D8, if that is even possible in the first place. If someone wants to investigate that, create an issue for it. If not, well, we're good.
Comment #49
fubhy CreditAttribution: fubhy commentedGood job Alex. That's indeed much cleaner.
Comment #50
katbailey CreditAttribution: katbailey commentedThis looks great - my main concern had been that you'd end up with a new container that wasn't the same one being referenced by the HttpKernel when managing scope. But I did a quick test calling
drupal_container()->isScopeActive('request')
after a request that enabled a module and everything seems to be in order, which has to mean thatleaveScope('request')
was called on the correct container. Nice one Alex! :-)Comment #51
webchickLooks like we have a winner! :) Great work, folks!
Committed and pushed to 8.x (with a minor tweak to remove the () from (array) in the @param docs in DrupalKernelInterface)
Comment #52
yched CreditAttribution: yched commentedNaive question, might very well be completely non-applicable: is there a way to rebuild the container only when needed, rather than force-rebuild for each module that gets enabled ?
I'm thinking of cases where you enable N modules at once, leading to N successive calls to kernel->boot().
I.e "setting 'menu_needs_rebuild' rather than calling menu_rebuild()" pattern.
Comment #53
tim.plunkettIIRC, views UI needs bundle services from views, so that wouldn't work for us, while this does.
Thanks to everyone who worked on this!
Comment #54
BerdirI have to reopen this one, as it caused a huge performance decrease.
It's the reason test runs now take 40++ minutes.
The changes in #512026-87: Move $form_state storage from cache to new state/key-value system from @effulgentsia seem to bring test runs back to 34 minutes but also seems to break the tests while doing so and leads to segmentation faults.
We should try to fix that in here I think, without the additional complexity added over there to see if those errors are caused by the keyvalue stuff or this.
Comment #55
effulgentsia CreditAttribution: effulgentsia commentedThe performance decrease is only in simpletest, installation, or other scenarios where module_enable() is called a lot. That seems like more of a major than a critical to me.
Additionally, what may have been uncovered in the $form_state issue is something similar to Kat's original concern in #50, where previously initialized services in the old container get destroyed in problematic ways (e.g., a service that another service's destructor depends on is destroyed before the dependent one's destructor is called) during initialization of a new container. This may warrant this issue being critical again once we have more understanding about what triggers the problem: at this time, I don't know yet why HEAD passes bot, but the patches in #512026: Move $form_state storage from cache to new state/key-value system manage to trigger the problem.
@Berdir: from #512026-85: Move $form_state storage from cache to new state/key-value system:
In the Drupal issue queue, we've been using the word "compile" to refer to two distinct operations. There's the
$container->compile()
step and there's the "dump to PHP file" step so that following requests are faster. It's already the case that we don't do the "dump to PHP file" step during installation (by not passing cache('bootstrap') to the kernel's constructor). The$container->compile()
step is semi-necessary though, because it's not just an optimization step, it also adds functionality to the container, like wiring up event subscribers. It's possible that our installation process is controlled enough that we can decide to lose out on all that functionality, but I'm not convinced it's worth it yet: rather, if the new DrupalKernel::updateModules() function is having nasty side effects by its implementation, it may be worth getting to the bottom of them and fix them directly.Comment #56
BerdirI think it doesn't really matter if it's critical or major, it being major is right now actually a bigger problem because we "only" have 11 critical bugs but 102 major ones :)
Thanks for the clarification on compile vs. dump. I agree that it makes sense to first fix (and actually understand) the problem and only then optimize it. Still, we're talking about a 30% performance decrease on the test execution time, that's serious.
Comment #57
Crell CreditAttribution: Crell commentedRelated: #1530756: [meta] Use a proper kernel for the installer
The goal is to make the installer its own micro-app, which would, hopefully, eliminate some of those issues. (It wouldn't help with enabling modules post-install, of course.)
Comment #58
BerdirRe-uploading a slightly modified version of effulgentsia's interdiff/patch from the form_state that should pass the tests, to see how much the difference is of this.
array_diff() is ... interesting.
array_diff($this->getBundleClasses($this->moduleList), $this->getBundleClasses($old_module_list)) is not the same as array_diff($this->getBundleClasses($old_module_list), $this->getBundleClasses($this->moduleList)).
What array_diff() seems to do is return the array values of the first array that are not in the second, but not the other way round. "$this->getBundleClasses($old_module_list) != $this->getBundleClasses($this->moduleList)" on the other side, seems to be working as expected.
Comment #59
tim.plunkettThis includes an issue I've found with old compiled containers being used when enabling, disabling, then re-enabling a module that provides a bundle.
It fixes failures in EnableDisableTest for the blocks-as-plugins patch.
I also added doc blocks.
Comment #60
tim.plunkett#58: only-reboot-kernel-if-necessary-1708692-58.patch queued for re-testing.
My change in #59 is obsolete now that #1831350: Break the circular dependency between bootstrap container and kernel is now in.
Comment #61
xjmI guess that #58 needs a reroll now.
Comment #62
BerdirI tried to do that but failed :)
As far as I can see, a similar check is now built into initializeContainer() and we do not attempt to rebuild if we either already have a dumped container (I don't quite understand yet how that plays together with $this->newModules ?) or the new modules differ from the old ones.
Maybe @chx can clarify...?
Comment #63
Fabianx CreditAttribution: Fabianx commentedAs discussed in IRC with berdir, the check inside of initializeContainer needs to be moved to the updateModules to not rebuild the container in case nothing has changed.
So this patch is still valid.
Comment #64
xjmAside from the not applying part?...
Comment #65
Berdirnot the patch itself is still valid, but the idea of it :)
Comment #66
BerdirOk, here is an attempt to do that. If we don't want that, because we also want to execute the second check ther, then an alternative would be to keep that helper function (added api docs from timplunkett's patch, thanks!) and use it for the existing check + make sure that the current container is only thrown away when we actually want to rebuild. Or so...
BundleTest passed, let's see about everything else.
Comment #68
Fabianx CreditAttribution: Fabianx commentedMinimal change, for testbot only:
This is definitely still wrong, but the logic is flawed for rebooting anyway:
At this point however drupal_container() contains our complete rebuild container ... (which will be merged in and not the Bootstrap Container)
I vote for a container_needs_rebuild flag in the Kernel, which updateModules can set and initializeContainer can use and then call drupal_container() with the right flag.
Comment #69
Fabianx CreditAttribution: Fabianx commentedfor testbot
Comment #70
Fabianx CreditAttribution: Fabianx commentedAnd another patch that might work better:
Comment #72
Fabianx CreditAttribution: Fabianx commentedAnd another patch
Comment #74
Fabianx CreditAttribution: Fabianx commentedGive me a green light ... please :)
Comment #75
Fabianx CreditAttribution: Fabianx commentedTestbot: I need another result, pleeeeeeease?
Comment #76
Fabianx CreditAttribution: Fabianx commentedReally, saving the moduleListChanged as a separate var is way cleaner and more inline with the original code, while still preventing the rebuild :-).
Comment #77
BerdirIs there a reason you left out the getBundleClasses() part? That allows us to only do a reboot if the changed modules actually are providing bundles.
Comment #78
Fabianx CreditAttribution: Fabianx commented#77: Yes, the getBundleClasses was not working and I was trying to understand the logic ...
Comment #79
Fabianx CreditAttribution: Fabianx commentedPatch with re-included getBundleClasses(), but new logic from #76.
Passes the relevant tests locally and seems faster. Lets see about the rest of Tests.
Comment #80
tim.plunkettIf you combine these two conditionals into one, then you don't need to indent. Otherwise this looks good. Do we have any benchmarks to prove it actually helps?
Comment #81
Fabianx CreditAttribution: Fabianx commentedGood catch! Done :)
How do you benchmark _that_?
Comment #82
sunThe issue title already states how this is to be benchmarked.
Unfortunately, the latest patch has zero impact on the total test suite time.
Comment #83
BerdirActually, it works just fine. It's just that it doesn't have a big impact anymore* and the testbots aren't a reliable indicator.
Confirmed with xhprof, see attached screenshot. Did run the same test with and without this patch, both had 7 calls to this function and without the patch, this resulted in 7 boot() calls. With the patch applied, only 3 calls to boot(). The inclusive wall time difference however is just 3% to 1,5%. And this is a relatively fast test (Actions configuration, first in the list) so the actual setUp() makes up a large amount of the total time (70%, to be exact).
I think this can be changed to priority normal, atleast the simpletest part of it. And we don't have a clear definition for the "other problems" except that the the additional container rebuilds (which are necessary) might or might not be causing my segfaults.
The only thing that could maybe be improved here is saving the bundle classes list of the current module list so that we don't have to re-check for that, which should remove half of the calls to getBundleLList().
*The reason for this is that the kernel patch that went in already had a check for exactly this, we don't really save much here except of an unecessary rebuild of the bootstrap container.
Comment #84
effulgentsia CreditAttribution: effulgentsia commentedI suspect what's happening is that when a container is rebuilt, the already instantiated instances inside the old container are not immediately destroyed. If there are many rebuilds in a single PHP request, then depending on how the instances reference each other, PHP is either able to garbage collect or not. If PHP isn't able to garbage collect, that could lead to an out of memory error. If it is, it might garbage collect, say the 'database' service prior to some other service that has a __destruct() that expects the 'database' service to exist. But I agree that all this is still supposition, and we have not yet confirmed it with clear reproducible steps.
Comment #85
chx CreditAttribution: chx commentedI am fairly confident that if we want to speed up simpletest then we need to save the list of profiles and as much of modules as we can to stop the astonishing amount of file scanning we do. This would require to remove the info handling from system_rebuild_module_data , or , indeed the removal of system_rebuild_module_data as it is... stay tuned, working on that.
Comment #86
andypostso what is needed here to fix?
Maybe this change would help to catch the actual errors that could be tested only manually for #1919002: Upgrade to D8 broken when D7 has more then one language enabled
Comment #87
dcam CreditAttribution: dcam commentedhttp://drupal.org/node/1427826 contains instructions for updating the issue summary with the summary template.
The summary may need to be updated with information from comments.
Comment #88
Pancho#2033567: Remove TestBase::rebuildContainer() is a followup on the original issue here, though it doesn't seem that "module enable/disable changes are immediately reflected in drupal_container()" as it was expected in TestBase::rebuildContainer().
Comment #89
PanchoWTH? I didn't remove that tag.
Comment #90
Pancho#81: core--only-reboot-kernel-if-necessary--1708692--81.diff queued for re-testing.
Comment #92
dlu CreditAttribution: dlu commentedReroll of #81.
Comment #94
dlu CreditAttribution: dlu commentedSeems that HEAD may be broken. On both my local dev machine and on simplytest.me I get this doing a fresh install of 8.x – I think that is why the patch fails.
This is the message I get:
Comment #95
Fabianx CreditAttribution: Fabianx commented#92: core--only-reboot-kernel-if-necessary--1708692--92.diff queued for re-testing.
Comment #97
Fabianx CreditAttribution: Fabianx commentedNope, 8.x works fine. The patch has a problem.
Comment #98
BerdirThat's because a lot has changed since that patch was written, multiple times :)
Bundle classes don't exist anymore, instead we have services.yml files and service provider classes. And more importantly, the container contains a list of module namespaces, used to look for plugins. I don't think what this patch is trying to do is still possible.
Comment #99
dlu CreditAttribution: dlu commentedMoved to simpletest.module per #2050763: Refine "base system" component.
Comment #100
xjm@Berdir made the case that this doesn't actually have anything to do with SimpleTest, so we agreed in IRC to move it back to base system for now.
@effulgentsia is also going to reassess this issue in light of the complete change to the affected architecture, so assigning to him. Thanks everyone!
Comment #101
tim.plunkettIs this still relevant?
If so, can we consider opening a new issue? This was committed in #51 (October 2012!), and it's hard to understand the current status of the issue.
Comment #102
effulgentsia CreditAttribution: effulgentsia commentedAgreed. Simpletest runs are about 40 minutes these days, which isn't that bad, and we can continue improving via conversions to PHPUnit and other optimizations. #84 might still be a theoretical problem, but there haven't been clearly reproducible steps reported, and if someone manages to find them, we can open a new issue for that. Therefore, restoring title and attributes of this issue prior to #54.