Race condition triggerable by a single user due to router rebuild in kernel.terminate

Adding patch here incase we do want to isolate that fix to here.

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.

Updated the summary slightly too.

I think we want to listen on both.
A) finish request is the right thing if you deal with an actual HTTP request, I totally agree with that
b) For drush you want to rebuild when drush is done, and afaik we fire the kernel terminate in drush
but not in drush.
So I would propose to add both events + keeping the ifNeeded()

Yes, that is a fair point. For regular HTTP requests, finish_request will get called, where as drush will only call terminate. So rebuildIfNeeded() will not be needed in terminate for HTTP requests as it's already called from finish_request. So it will just be a NULL op.

So the options are implement both (meh), or drush also implements the finish_request event also.

The later one would be wrong though, IMHO

Straight rebase b/c patch wouldn't apply.

And a patch that responds to both.

This still doesn't help if the entire service is not registered in the core.services.yml file.

Talked w/ @dawehner briefly about this in IRC and will post a more better version tonight.

Ok, service is back.

#2164367: Rebuild router as few times as possible per request added it.

#356399: Optimize the route rebuilding process to rebuild on write removed it.

Do we need a test? Is this really testable?

Do we need a test? Is this really testable?

I'm pretty sure we can test this with a kernel test.

Did you see #2564921: In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page, as this was originally created based on that. The two containers is a real issue so I don't think we gain anything from doing this yet?

The latest patch does not match the issue title.

Added an issue to at least try to explore the ideas around separating rebuilding in its own step: #2600028: Try to rebuild the container after module install in its own controller

Retitling further since moving from TERMINATE to FINISH_REQUEST doesn't fully solve the race condition described in #2572285: Module enabling and router rebuilding should be done in one transaction, only the single-user race condition described in this issue's summary, which is still progress.

Could this be added as a 8.0.2 target?

Please stop changing this. Thanks.

Hi

This issue is easy to trigger using the Core Search pages config.

Go to Configuration > Search pages, add a new page, save. You are expected to be redirected on the new search page, instead you get an "unknown route" Fatal error.

Now that RouterRebuildSubscriber is gone with #2613400: Remove Drupal\Core\EventSubscriber\RouterRebuildSubscriber do we still have this issue actually?

And to answer @dawehner, I'm still able to reproduce this with the lastest 8.3.x