In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page

I'm unable to reproduce this against HEAD using the steps outlined in the IS. When enabling the statistics module immediately after installation, and after uninstalling/re-enabling the statistics module, everything appears to work as expected, no errors are thrown and I can access the modules settings page.

I have PHP-FPM too and I can reproduce.

Updated IS with #5 informations.
Added beta eval.

Updated IS and title with more details.

Backtrace asked on IRC.

Changed the title to point that this is relevant for PHP-FPM. Also, tagging, and changing subsystem to routing system (not only statistics module is affected)

It seems that I am only able to reproduce this using PHP-FPM.

So the fact why this just happens on PHP-FPM is that we reproduce the router now on kernel.terminate.
On FPM this happens after the response was sent out to the client, in which case the router is rebuilt afterwards. The browser in the meantime redirects to the page and at that page load the route is not there yet.

On the other hand I don't see why this should be a critical issue. If you hit f5 again everything will be alright.

Its kinda by design how we rebuild the router at the moment.

@catch and @dawehner talked about that on IRC
The assumption first was that the configure links for the modules are broken but indeed the toolbar is causing problems.
Its not really clear how the menu rebuild can be done before the router rebuild is done, given that the menu rebuild is coming 100% after the router finished, see
\Drupal\Core\Routing\RouteBuilder::rebuild ... this is confusing.

While browsing through some code I found \Drupal\page_manager\Plugin\PageManagerMenu\NormalMenu::invalidate
doing its own menu link rebuilding, but I cannot imagine how this could exactly cause the issues described here. Do you have page_manager installed on that particular site?

Following @dawehner's instructions I tried to put some traces at the beginning and the end of these functions:

• MenuLinkManager::rebuild()
• RouteBuilder::rebuild()
• _toolbar_do_get_rendered_subtrees()

The result follows:

$ cat /tmp/MenuLinkManagerRebuild 
1441728448.2029 Drupal\Core\Routing\RouteBuilder::rebuild
1441728448.602 Drupal\Core\Menu\MenuLinkManager::rebuild
1441728448.7894 Drupal\Core\Menu\MenuLinkManager::rebuild END
1441728448.8055 Drupal\Core\Routing\RouteBuilder::rebuild END
1441728449.2114 _toolbar_do_get_rendered_subtrees

Note: _toolbar_do_get_rendered_subtrees() crashes before it reaches its "END" trace.
Note 2: @Haza told us on IRC that adding a sleep(5) on the first line of MenuLinkManager::rebuild() "fixes" the issue.

If someone wants to continue here is the patch that adds the traces.

Update: issue happens only when toolbar module is enabled

First take on solution. But I have no idea how (and if) we can test the issue.

@valthebald Thank you for your patch! Just one question:

+++ b/core/lib/Drupal/Core/Extension/ModuleInstaller.php
@@ -289,7 +289,9 @@ public function install(array $module_list, $enable_dependencies = TRUE) {
+      $builder->setRebuildNeeded();
+      $builder->rebuildIfNeeded();

Why don't you directly call $builder->rebuild() here?

@DuaelFr: no reason, I've left $builder variable just as a leftover from other things that I tried to do. Anyway, testbot didn't like this approach, as it breaks other things.

Another possible solution is to try and rebuild routes in RouteProvider, giving the final chance before throwing RouteNotFoundException.

Check may be placed in RouteProvider::getRouteByName():

  public function getRouteByName($name) {
    $routes = $this->getRoutesByNames(array($name));
    if (empty($routes)) {
      // Rebuild routes
      \Drupal::service('router.builder')->rebuild();
      $routes = $this->getRoutesByNames(array($name));
      if (empty($routes)) {
        throw new RouteNotFoundException(sprintf('Route "%s" does not exist.', $name));
      }
    }

    return reset($routes);
  }

or, as alternative, it is possible to catch RouteNotFoundException in ModulesListForm::buildForm():

      try {
        $checkedRoute = $this->accessManager->checkNamedRoute($module->info['configure'], $route_parameters, $this->currentUser);
      }
      catch (RouteNotFoundException $exception) {
        // Rebuild routes
        \Drupal::service('router.builder')->rebuild();
        $checkedRoute = $this->accessManager->checkNamedRoute($module->info['configure'], $route_parameters, $this->currentUser);
      }

I personally prefer the second method, since it's less disruptive. Let's see what testbot thinks

Grrr, cross-posting made me lose all my comment :/

I would suggest, as it is a toolbar issue, to fix it in the toolbar module. The easiest would be to implement hook_modules_installed() as it'd have the less disruptive impact. The cleanest would be to find somewhere in _toolbar_do_get_rendered_subtrees() to call RouteBuilder::rebuildIfNeeded().

About #27: I think that you should use rebuildIfNeeded() in your two options because it spends less resources in case the route is not defined at all and it's not just a rebuild issue.

index 0a52d2f..a36a790 100644
--- a/core/modules/system/src/Form/ModulesListForm.php

--- a/core/modules/system/src/Form/ModulesListForm.php
+++ b/core/modules/system/src/Form/ModulesListForm.php

+++ b/core/modules/system/src/Form/ModulesListForm.php
@@ -27,6 +27,7 @@

This is what I don't get, the backtrace has shown the toolbar

I was able to reproduce the issue with toolbar module switched off, my comment #19 was wrong. More correctly, with toolbar module the issue is always reproducable, without it happens randomly.

I was able to reproduce the issue with toolbar module switched off, my comment #19 was wrong. More correctly, with toolbar module the issue is always reproducable, without it happens randomly.

So toolbar is actually a different problem we need to fix on top of that?

+++ b/core/modules/toolbar/toolbar.module
@@ -295,6 +295,11 @@ function toolbar_get_rendered_subtrees() {
+  // Ensures that the router has been rebuilt after a module has been installed
+  // to avoid a Fatal error while trying to show a link to the module's
+  // configuration route defined in its info file.
+  \Drupal::service('router.builder')->rebuildIfNeeded();

MHH it seems to be that all of that are just workarounds, do we actually understand for toolbar how it could happen?

I don't ;)

I've updated issue description with what (IMO) leads to the issue - "What happens" section.
toolbar module, while not being direct cause, can make the issue more reproducible in 2 ways:

1. it adds $kernel->terminate() task that makes the first request longer
2. it has an early call to RouteProvider::getRouteByName(), that crashes the second request

I cannot prove that theory though

I was able to dig some further. I placed traces of current request time (float) in the following pieces of code:

• ModulesListForm.php::buildRow()
• RouteBuilder::build()
• RouteProvider::reset()
• RouteProvider::getRouteByName()
• AccessManager::checkNamedRoute()

Here are results of enabling Actions module under different conditions:

mod_php, toolbar enabled

1441880274.303 rebuild in process Drupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:195
1441880274.303 cache reset Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::reset:390
1441880274.303 rebuild finished Drupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:206
1441880276.556 Drupal\system\Form\ModulesListForm:Drupal\system\Form\ModulesListForm::buildRow:303

Process finished successfully, no errors. First request (1441880274.303) finished before form is finished to user

FPM, toolbar disabled

1441880731.3191 Drupal\system\Form\ModulesListForm:Drupal\system\Form\ModulesListForm::buildRow:303
1441880731.3191 Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::getRouteByName:194
1441880731.3191 exception caught Drupal\Core\Access\AccessManager:Drupal\Core\Access\AccessManager::checkNamedRoute:101
1441880729.849 rebuild in processDrupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:195
1441880729.849 cache reset Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::reset:387
1441880729.849 rebuild finished Drupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:206

Process finished successfully, no errors. Second request (1441880731.3191) throwed RouteNotFoundException, which was caught in AccessManager::checkNamedRoute(). Actions module line in modules list doesn't have configure link. After refresh, configure link is present

FPM, toolbar enabled

1441881096.3129 Drupal\system\Form\ModulesListForm:Drupal\system\Form\ModulesListForm::buildRow:303
1441881096.3129 Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::getRouteByName:194
1441881096.3129 exception caught Drupal\Core\Access\AccessManager:Drupal\Core\Access\AccessManager::checkNamedRoute:101
1441881094.5781 rebuild in processDrupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:195
1441881094.5781 cache reset Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::reset:387
1441881094.5781 rebuild finished Drupal\Core\Routing\RouteBuilder:Drupal\Core\Routing\RouteBuilder::rebuild:206
1441881096.3129 Drupal\Core\Routing\RouteProvider:Drupal\Core\Routing\RouteProvider::getRouteByName:194

Process crashed with unhandled exception. Second request (1441880731.3191) throwed RouteNotFoundException, which was caught in AccessManager::checkNamedRoute(). After that, first request finished rebuilding router, second request throwed RouteNotFoundException for the second time, which was not caught

Here's the patch that shows the problem. Test should fail.
I am not sure where this test belongs. Since it needs to enable some module, I don't think it can be placed at Drupal/Tests/Core/Routing. My guess is Drupal\system\Tests\Routing\RouteBuilderTest.

Uncommenting line 28:

      //$this->rebuildAll();

makes the test pass

Testbot hiccup?

My bad, wrong annotation and class name for the test.
No other differences from #38

First of all, the patch in #46 fixes the issue, thank you for that.
Now, I have a serious doubt about the way it's done.

With this patch, each time the UrlGenerator::getRoute() method is called on a non-existent route, it rebuilds the entire routes registry. In some cases that could be a huge performance problem (imagine a script that loops on expected routes to find a good one). I think we should only rebuild that registry once per request.

I tried to use rebuildIfNeeded() instead of rebuild() but it does not fix the bug anymore.

@DuaelFr: I have the same performance concern.
Unfortunately, we cannot use check inside rebuildIfNeeded() - it relies on the value of static variable $rebuildNeeded, which scope is only the current request. What we need is a state variable shared between route builder and route provider. Maybe we can use state service for that purpose?

That is what I was thinking too.
I think we should ask some specialist in the critical team to give us his/her opinion.

OK, here's the change. I've added state service to RouteBuilder constructor, changed logic of RouteBuilder::rebuildIfNeeded() - to use state service instead of static variable.

It made sense to me to move logic of giving final chance to RouteBuilder from UrlGenerator to RouteProvider.
No interdiff since it's different logic compared to the last patch

Me and catch were going through the code and rather that try to just catch the exception, which could lead to problems discussed above. As well as coding around kind of specifically for a case like this.

We could consider just moving the rebuild event subscriber to use FINISH_REQUEST instead of TERMINATE. As this will always run before the response is sent (See HttpKernel::filterResponse()), terminate is called after the response is sent.

Whoops.

Opened #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate #2572285: Module enabling and router rebuilding should be done in one transaction and #2572283: Neither REPEATABLE READ nor READ COMMITTED transaction isolation levels are always appropriate for the discussion, we could mark the kernel.terminate issue as duplicate of this one, couldn't find it at the time.

catch, do you want me to move this patch into #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate then?

Unfortunately, patch from #58 does not fix original problem

That's why we split it into the child issue now. It feels like the patch in #52 is kind of wrong. Couldn't two processes be trying to rebuild the router in that case?

@damianklop: it looks like RouterRebuildSubsciber needs also this addition to core.services.yml (code was not executed until I added it)

router_rebuild:
    class: Drupal\Core\EventSubscriber\RouterRebuildSubscriber
    tags:
      - { name: event_subscriber }
    arguments: ['@router.builder']

Yes, good spot. This is not even running. I am wondering what we are doing with this subscriber in that case!

I've slightly modified the patch from #62:

1. Added event listener to core.services.yml
2. Replaced $this->routerBuilder with \Drupal::service('router.builder')

Without the second change, issue is still there. ModuleInstaller calls \Drupal::service('router.builder')->setRebuildNeeded() *before* RouterRebuildSubscriber::onKernelFinishRequest(), but RouterRebuildSubscriber::routerBuilder->isRebuildNeeded is false during that call.

I think we need to work out why this subscriber is not used but still in core now.

@damiankloip: this code from KernelDestructionSubscriber:

  public function onKernelTerminate(PostResponseEvent $event) {
    foreach ($this->services as $id) {
      // Check if the service was initialized during this request, destruction
      // is not necessary if the service was not used.
      if ($this->container->initialized($id)) {
        $service = $this->container->get($id);
        $service->destruct();
      }
    }
  }

destructs RouterBuilder service, which calls rebuildIfNeeded(). So yes, in the current HEAD RouterRebuildSubscriber is not called, so it's just a ballast code.

If we move RouterRebuildSubscriber to react to FinishRequest, KernelDestructionSubscriber still will work, but rebuild will not happen for the second time (because rebuildNeeded==FALSE)

I found a way to reproduce this in page_manager (#2301485: Fatal error: Call to a member function getOption() after creating a new page). Using PHP-FPM on a fresh D8 site with page_manager, I would get an error when creating a new page because the page edit form was trying to load before the Route rebuild had finished saving to the DB. This is because RouteBuilder is doing the call to rebuild in destruct. Using the patch in #66 fixed the error. It causes the rebuild to happen earlier before the edit form is loaded and needs to access the routes.

Adding #66 fixed the race condition. Removing #66 brought it back again. So I'm fairly confident I had a good test system for reviewing this issue and feel pretty safe marking this RTBC.

Well done mates, it's a really nice catch.
Thank you allowing me to discover that Kernel Terminate event and how symfony deal with php-fpm. I now understand one of the reasons why php-fpm has such good performances! :)

We should have had some test coverage break due to this not getting run, let's add some here.

Also are there any places we're forcing a rebuild but could stop doing so now this is fixed?

I'm also quite convinced that for drush we should keep kernel terminate, because this is executed there, but finish request just doesn't make sense, if you ask me.

@dawehner: kernel terminate cleanup task is run anyway, because rebuildIfNeeded() is part of RouterBuilder destructor.

@catch: what is the best way to test if event listeners is being called? I haven't done such a thing yet

I think we should have one or the other, as we need finish_request too, it makes sense for the RouteBuilder class not to implement destructable interface and just use one subscriber? HOWEVER, destruction will check if the service was used. So this change I guess could be problematic as it would always instantiate a RouteBuilder service regardless. Which is potentially not good.

Maybe subscriber can check if the service was initialized

Attached (new) test that checks that finish request listener is called, and finish request listener checks if router builder service was initialized

OK, I can confirm the following:

1) With the patch from #66 NOT applied, but using the test from #78, the test correctly fails.

2) With the patch from #66 applied, the test in #78 passes.

So this looks good. The test reproduced the problem and correctly fails on a PHP-FPM system.

Thanks for testing, that is some much needed validation. However, I think we still need to do a little bit of work. We currently implement DestructableInterface and have the subscriber, with the subscriber mimicking what destructable functionality provides (checking whether the service has be instantiated).

I think we need to either:

1. Remove DestructableInterface from RouteBuilder, and add kernel_events.terminate in the RouterRebuildSubscriber so both events are implemented there.
2. Either extend Destructable, or implement something similar that will provide this functionality but run in kernel_events.finish_request.

@damiankloip: shouldn't this be addressed in a separate issue #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate?

It could do, but it seems more sensible to just do it all here, and have everything verified as working together. Or there, I don't mind. The final solution here would affect that anyway. That issue was going to be for the actual fix, but you kept working in here, so we stayed in here :)

Let's see what testbot says for removal of DestructableInterface
@damiankloip: why do you suggest to add kernel_events.terminate in the RouterRebuildSubscriber? I thought one of the goals is to *not* rebuild on kernel terminate

Well. Things like drush will still rely on terminate, as stated somewhere above or the referenced issue. I forget now. If it happens in finosh request terminate is a null op on a regular request so no harm done.

Also, I don't think the change in the subscriber to use \Drupal:: to get the route builder will fly...

@damiankloip: if we need both finish request and kernel terminate, what's the point to remove destructable interface?

@damiankloip: regarding your doubt about route builder - well, apparently it works, according to both test and the fact that the patch solves the problem :)

Apparently, removing DestructableInterface is not a small change... There are lot of tests relying on RouteBuilder::destroy()

Anyway, I'm still confused by (IMO controversial) needs to remove DestructableInterface and not rebuild router on kernel.terminate

Since we call rebuildIfNeeded() on the same object on both finish request and kernel terminate events, router is not rebuilt for the second time, so there is no performance impact here

It's not about the performance impact. I understand how this is all working as I suggested the fix originally... and outlined the need for both. The issue I see is that the subscriber is being added back, made container aware, and replicating functionality of destructible interface. So having these things in two places makes things more confusing.

I would be very surprised if this patch got committed in its current state though.

@damiankloip: I still don't understand how do you suggest to address that. Can we discuss this briefly in IRC?

The problem with "doing it all here" is that we are risking this not getting fixed anytime soon. Can we work iteratively here with multiple issues instead of blocking this till it's perfect. I'm very concerned about problems people will have with the RC release on PHP-FPM. Doing it in two places seems a lot better than doing nothing at all and having people getting fatal error messages.

e.g., trying to work on Page Manager for D8. This issue causes creations of new page routes in the UI to fail on systems with PHP FPM "sometimes" which leads to a lot of head-scratching and time consuming debugging and confusion.

Yes, appreciate the need to get something in. But there is a line to just getting whatever in. I'm afraid at this late stage in the cycle, that doesn't wash quite as easily. IMO this needs to be done in one go.

Uhm, this was a big performance improvement to lazy build the router we are removing here.

I think the biggest problem - besides the router being dirty - which is a totally different problem - is 302 responses for the day-to-day use-case.

We already use our own class for redirects, so we could be using a ResponseEmitterInterface (Something like #2577631: Allow HtmlResponse to use a flexible emitter) to control the send() function from a service.

We could then check if any dirty laundry operation is in progress for terminate and choose to not call fastcgi_finish_request() and instead wait().

On the other hand, then we could have waited all along ...

This is not removing the laziness I don't think. It just replicates what DestructableInterface is doing. The response emitter idea is similar to something me and catch were talking about before, extending the response to not call fastcgi_finish_request(). This is definitely an interesting idea!

The thing that is potentially worrying is that another modules doing something similar will also need to do whatever we do here. Which may not be obvious at all.

The underlying issue is still here, that terminate is not meant to be used for this type of thing, finish_request is. I think.

Talking with dawehner, the most easy change is to use a very late FinishResponseSubscriber and do:

if (in_array(Response->getStatusCode(), [301,302]) && $this->router->isRebuildNeeded()) {
  $this->router->rebuild();
}

That is a around 20 loc code change that even contrib could implement and would solve the problem for the 302 to the new page for views and page manager in the least invasive way for PHP-FPM.

2 notes:

1. Redirect codes array has to include 303 See other (see #355157: 302 response code from drupal_redirect_form() violates HTTP/1.1 spec)
2. $this->router has to be replaced with \Drupal::service('router.builder') - as I mentioned in #66, using $this->router doesn't solve the problem

something like this?

if (in_array(Response->getStatusCode(), [301, 302, 303])) {
  \Drupal::service('router.builder')->rebuildIfNeeded();
}

Do you know why that is the case? Why \Drupal:: has to be used? Is this because the container is being rebuilt? Because that certainly seems like a big red flag, as previously mentioned.

Luckily (no big red flag for now), this is not related to container being rebuilt, rather to the fact that service is lazily loaded. What happens is:

1. RouterRebuildSubscriber constructor gets RouteBuilder proxy as its parameter
2. ModuleInstaller calls Drupal::service('router_builder')->setRebuildNeeded()
3. RouteBuilder is instantiated, and replaces its proxy in services list
4. On finish request, RouterRebuildSubscriber uses $this->router_builder (proxy!), that creates a new RouterBuilder object

IMO, execution flow makes no sense in having @router.builder as a parameter of RouterRebuildSubscriber constructor

$this->router->rebuildIfNeeded() or \Drupal::service('router.builder')->rebuildIfNeeded() will instantiate RouteBuilder. #78 uses $container->initialized() to do it only when necessary.

On finish request, RouterRebuildSubscriber uses $this->router_builder (proxy!), that creates a new RouterBuilder object

IMO, execution flow makes no sense in having @router.builder as a parameter of RouterRebuildSubscriber constructor

Hmm, that seems to be quite a big problem generally. It definitely makes sense for the subscriber to inject the route builder service, as otherwise we are just falling back for Drupal globals again.

RouterBuilder already treats $this->rebuildNeeded as a global. This issue just makes it more visible.
OK, everybody agrees using globals is bad. Should this meta be fixed in *this* issue? I doubt so.
Why can't we fix this (very major - good definition) issue, and move forward? Creating follow up issues (or linking existing) is fine, but I don't see why it should stop this issue from being fixed.

For the record: making rebuildNeeded static variable of RouterBuilder class makes usage of $this->router possible in RouterRebuildSubscriber. Of course, it still leaves the problem of loading of RouterBuilder class only when it's necessary

I agree with #104: I think there is no way around using the container aware approach that DestructableKernelInterface uses.

Sorry for the slight distraction.

I don't think there is an issue making this container aware, I think this outlines a bigger issue that only \Drupal::service('route.builder') works within the subscriber as the container is changed mid request!

$rebuildNeeded is not global, it's just a property on the RouteBuilder instance.

Opened #2589967: Rebuild routes immediately when modules are installed.

@catch: as far as I see, there is a bunch of stuff happening before RouterBuilder::setRebuildNeeded() and actual rebuild. I also reckon that I when tried to call RouterBuilder::rebuild() right after module is enabled, this broke quite many tests. Do you see any flows in approach of #78? The patch solved the problem, worked, and passed all the tests

what's important, #78 is less disruptive than initial patch of #2589967: Rebuild routes immediately when modules are installed

What's worrying about that is it only works with \Drupal::service () because it uses a different container to the one the event was dispatched from. This makes things very weird.

@damiankloip: is it the same question that you asked in #100? If so, I've answered in #101 - no, it's not because different container is used, it's because RouterBuilder service uses non-static variable rebuildNeeded, which is always set to FALSE in constructor.

EventSubscriber gets proxy $router_builder in constructor => ModuleInstaller initializes router.builder service => ModuleInstaller sets rebuildNeeded in RouterBuilder instance that is stored in container => EventSubscriber reacts on finish request event => EventSubscriber gets new instance of RouterBuilder => new instance's rebuildNeeded is FALSE => new instance's rebuildIfNeeded does nothing.

This flow pretty much explains why \Drupal::service solves the problem - because it doesn't create another instance of RouterBuilder.

What's weird in this explanation?

@valthebald the patch here solves the specific bug with php-fpm that we do things in terminate thst must complete before a redirect is finished.

It does not solve the issue that we have a race condition between module install and router rebuild - a second tab hitting the modules page could triggered the same error here even with this patch applied.

it's not because different container is used, it's because RouterBuilder service uses non-static variable rebuildNeeded, which is always set to FALSE in constructor.

EventSubscriber gets new instance of RouterBuilder

And why do you think you get a new instance of the route builder the second time?! Because you have a new container. If it was the same container why would you get a new instance? The proxy would give you the same instance as the CONTAINER is injected in there.

So what you are explaining is the exact symptom of there being two containers, hence the need for you to use \Drupal::service(). This will use the router builder from the new container, where are injecting it AND the event subscribers are attached and invoked from the old container.

So it's not your explanation that's weird, it's the conclusion.

Hi there, I'm sorry to have left this thread without love for such a long time.
I suggest that everyone take a deep breath and calm down. You are all doing great job here and on Drupal in general so let's keep the head out of water and smile :)

So, can we focus on an acceptable fix and let a todo (an open a follow-up) to fix the deeper problem later?
As a lot of hosters are running on php-fpm, we need that immediate problem to be fixed quickly. I agree that the container should not be rebuilt but no one seems to yell for this particular issue so that can wait a bit.

I think our best option for now is to use a static variable to mark the router builder as needing a rebuild AND move this call to the finish request event instead of terminate. Let's do this and move on!

There is no issue with using finish_request here, I was the one that suggested that originally instead of the original locking fix.

The trouble I see is that we have to use \Drupal::service('route.builder') from the event subscriber because the event is invoked from container A but module handler uses container B. So this fix to use the global method to get the current container just seems like a hack.

And as catch says, it only really fixes the problem for one case, enabling a module via the UI with FPM. Yes, quite a big problem but this is a general problem we have. Rebuilding the container during the cycle of a request is bad™

First, I need to apologize if my comments hurt anyone. This issue is stuck with no visible progress for quite some time, so I maybe gave unnecessary emotions outbreak.

Second, is #2589967: Rebuild routes immediately when modules are installed is committable any time soon? @catch mentions race condition in #113, but was it ever spotted in the wild? When I was thinking about initial test, I performed multiple debug sessions, stopping execution of module install at different stages, and tried to reproduce WSOD by opening module list in the second browser tab - and module list in the second tab always showed up correctly. So the patch can solve bigger problem, than described in this issue. This is true. But it also for sure makes tests slower, and break other things. All in change for what it seems like theoretical problem solution. Do we want to do it before 8.0.0?

Third

Rebuilding the container during the cycle of a request is bad™

nobody argues with that. But how is that related to the issue?

Fourth, to sum up:
this issue (and last known-to-fix-the-issue patch #78) itself is not about container rebuild, or router rebuild. It's about very specific fact that FPM request continues execution after data is sent to client (all heil FPM power). Of course, if #2589967: Rebuild routes immediately when modules are installed was already committed, this issue would be irrelevant. If container was not rebuilt during request life cycle, there would be no need in using \Drupal::service().

I would be happy (for the sake of solving this specific issue) if router was rebuilt immediately. I would be happy if container was not rebuilt during one request cycle. None of the last things is true. So what do we do?

My suggestion is the following :
- move the call to rebuilIfNeeded in the finish request event
- use a getter and change the setter to rely on Drupal_static in the router builder and ensure the rebuildNeeded flag is consistent across multiple instances
- add a todo comment to remove that trick once we are sure the container is not rebuilt during the request

Sadly, I don't think I'll have the time to propose the associated patch.

Here is an half-working patch following my suggestion in #118.
It fixes the issue in my fpm environment.

I need some help with the following :
- add the todo comment and improve comments to explain why we use drupal_static (as a non-native speaker I have some difficulties to write that king of thing)
- fix the unit tests (the calls to drupal_static throw errors and I don't know how to bypass them)

Is this DS issue related? https://www.drupal.org/node/2594983

@aspilicious: it looks so!

I just wanted to double check if anything has been committed for this. I hope 8.0 isn't released with this bug?

I just manually tested on HEAD and the answer in "no".

The issue is too big to find its way into 8.0.0, given known release date. But we should try and solve the problem of late router rebuild in 8.0.1 (no need to wait until 8.1 or 9.x)

I know you guys do amazing work, and I always appreciate it, but I have to say, this is going to be such a common scenario, people are going to be running into this bug a lot when 8.0 is released.

IMHO, a WSOD in a common use case should have been marked critical, and a 8.0 release blocker.

At this point this is a duplicate of #2572293: Race condition triggerable by a single user due to router rebuild in kernel.terminate and #2589967: Rebuild routes immediately when modules are installed. The first issue is a quick fix (similar to the last patch here) that could probably land before 8.0.0 if it's ready, but it needs test coverage and probably manual testing on php-fpm.

In general, issues tend to get fixed faster when patches get reviewed and test coverage, rather than by asking when they're going to be fixed.