Route rebuilding is not guaranteed to finish in time for the next request

This should be it.

I manually tested this patch, and it fixes #2229303: Enable/add of search pages leads to route not found exception.

Sure, let's do it.

The reason for this failure is the following problem: People call getRouteByName during route rebuildling. (rest (yes rest!), config_translation),
which result in infinite recursion.
This is really fragile if you change anything. #2158571: Routes added in RouteSubscribers cannot be altered solves a couple of those problems,
by passing along the route collection during route rebuild time.

There are good reasons for people calling getRouteByName (directly or indirectly) during dynamic route creation, for instance so that they can base their URLs on existing static routes. Presumably they wouldn't need the dynamic routes for this?

I disagree that you should do that. It was indeed just implementation detail that this kinda worked in most cases currently.
In the linked patch you have events you can use which retrieve all the previous defined routes, just not accessible via the route provider interface.

That sounds like a good idea. This should be documented on the getRouteByName and other methods that shouldn't be called during dynamic route creation.

If things would be always simple ...

Looks critical to me.

MatcherDumper::dump() is supposed to do this in a transaction to avoid the empty router table issue. This issue proves that we can't rely on transactions for that - likely default transaction isolation level isn't enough to protect against this.

https://api.drupal.org/api/drupal/includes%21menu.inc/function/menu_get_... had a check for menu rebuild as well as an empty menu masks variable.

menu_rebuild() had a lock to prevent stampedes.

Looks like both of those need equivalents adding back.

Well, the router rebuilder has a lock.

Patch no longer applied, needed reroll. RouteProviderTests pass locally

This patch still applies, but much has changed in this area.. so I am about to retest.

Retest wipes away test data so I am providing a link to it here, currently 4 fails, 1 exception

https://qa.drupal.org/pifr/test/805923

@martin107
If you re-test a patch you can't really access previous results. In case these errors are helpful consider to just reupload a patch.

posting the backtrace which lead to the current exceptions:

There seems to be something going on wrong during rebuild of the routes.

Drupal\Core\Routing\RouteProvider->getRouteByName('system.admin_content')
Drupal\Core\Routing\UrlGenerator->getRoute('system.admin_content')
Drupal\Core\Routing\UrlGenerator->generateFromRoute('system.admin_content', Array, Array)
Drupal::url('system.admin_content')
node_permission()
call_user_func_array('node_permission', Array)
Drupal\Core\Extension\ModuleHandler->invokeAll('permission')
Drupal\simpletest\WebTestBase->checkPermissions(Array)
Drupal\simpletest\WebTestBase->drupalCreateRole(Array)
Drupal\simpletest\WebTestBase->drupalCreateUser(Array)
Drupal\comment\Tests\CommentStatisticsTest->setUp()
Drupal\simpletest\TestBase->run()
_simpletest_batch_operation(Array, '8', Array)
call_user_func_array('_simpletest_batch_operation', Array)
_batch_process()
_batch_do()
_batch_page(Object)
Drupal\system\Controller\BatchController->batchPage(Object)
call_user_func_array(Array, Array)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Sadly this is kind of similar as the failure in #2327965: drupal_flush_all_caches() doesn't clear all plugin caches

I won't trigger another retest ... but just to highlight this extract from the test errors.

Allowed memory size of 335544320 bytes exhausted (tried to allocate 130968 bytes)

Yeah this is probably caused by some recursive call of the route rebuilding I can't really figure out yet.

mgifford - this issue was already marked "related" from that other one. Not really necessary to have it in the sidebar twice... :)

Sorry, thanks for removing it. Most postponed issues I was looking at weren't properly related to why they were postponed. I definitely made a few mistakes.

It fixes #2229303. Rerolled. Let's take a look at the test results.

Architectural question:

- Isn't it much simpler to decrease the priority of the rebuild to happen after the redirect.

Then check if a rebuild is needed on route request (like done in D7 with menu_reubild_needed()) - need to do that anyway as a process can die at any time ...

Trying a combined patch of this an #2248297: Ensure routes are rebuilt when install modules, just to see what happens.

Ok, back to the previous patch. Found the problem, it was a recursion in config_translation.module.

Fixed that, checked with @tstoeckler, we no longer need that. Also added a recursion protection so we have something more useful to look at.

+++ b/core/includes/common.inc
@@ -860,7 +860,7 @@ function drupal_set_time_limit($time_limit) {
     if ($current != 0) {
-      @set_time_limit($time_limit);
+      //@set_time_limit($time_limit);
     }

This is probably a debugging oversight?

Besides that: RTBC - Nice work!

This is touching this area of code, so adding related issue of #356399: Optimize the route rebuilding process to rebuild on write.

Yep.

RTBC, then!

Looks great. Was wondering if we need to document more why we throw the exception, but can't really think of succint docs for that.

Committed/pushed to 8.0.x, thanks!

I get the new exception triggered on module enable / drush cache-rebuild. See #2351353: What to do with "Recursive router rebuild detected."

PHP8.0, Drupal 9.3.16
- Install with standard profile.
- Go to Configuration > Search and Meta Data > Search pages (admin/config/search/pages)
- Add a new Content search page.

Symfony\Component\Routing\Exception\RouteNotFoundException: Route "search.view_test" does not exist. in Drupal\Core\Routing\RouteProvider->getRouteByName() (line 206 of docroot/core/lib/Drupal/Core/Routing/RouteProvider.php).