fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely together

Create a service to register shutdown functions, which run before sending the response

@fabianx: rather than "before sending the response", should this not be "before the connection to the client is closed", that is, before symfony does it's fastcgi_finish_request.

Because we want the response to be sent ASAP. The user can have the feeling the page is loaded, even if the connection is still open. This is not true, though, depending on the type of response and will only work for HTML pages. Redirects, Ajax and other get totally blocked until the request has 100% finished and connection is closed.

I understand that the race condition you mention only happens in environments where fastcgi_finish_request() does work.

I have a feeling there's already an issue open for this.

I'd be tempted to override send() to get rid of the fastcgi_finish_request(), unless someone can prove it has a genuine benefit for page serving which seems unlikely compared to BigPipe etc.

What happens is this: batch step 1 starts - batch step 2 starts - batch step 1 updates the {batch} table in shutdown. One symptom is that the single config import form gives you a broken error message "An error occurred while processing with arguments:" but the import actually succeeds. Other batch functionality might be more broken.

? (Emphasis added)

I was very confused by this, because I didn't write the single config import form to use a batch.

But this issue added it: #2486467: Single config import form needs to use the config validation events

Not super helpful, sorry.

This is patch that is mix of the 2 proposed solutions.

Here is patch that

1. Overrides send() in our local inheritance of Response.php (used now only in HtmlResponse but probably should be used in CacheableResponse, JSONResponse, etc.)
2. Creates a ShutDownRegistry service to register shutdown functions. When registering a shutdown you can set $prevent_early_flush.
3. The inheritance of Response.php used the ShutDownRegistry to check preventEarlyFlush().
4. Replace call to drupal_register_shutdown_function in batch.inc to use ShutDownRegistry(all other calls would be replaced and drupal_register_shutdown_function deprecated)

@todo need to pass ShutDownRegistry service in the Response.php constructor.

I have manually tested batch_test page and does bypass fastcgi_finish_request().

Sorry if this totally off track just getting a handle on how this works.

Instead of using the ShutdownRegistry service in the Response class this passes an optional $prevent_early_flush bool to the constructor. The classes the instantiate a response object that extends this will use the Shutdown service if needed. Updated the 2 instances of using the HTMLResponse that I saw.

If we want to push the change up stream to Symfony we shouldn't rely on our service in the Response.

@Fabianx well let's not forget #2564921: In PHP-FPM environment, enabling a module in using a 'configure' route leads to an error page / #2589967: Rebuild routes immediately when modules are installed. We have to be certain that background work isn't needed before the next request by the same user, and core itself was not doing that correctly.

Also we have a background API, it's the queue API, so if things don't need to be done before the request finishes that's fine. Then we can support waiting queues or add something like #1189464: Add an 'instant' queue runner, but if this is a critical bug, let's fix the bug, not add new features based on the bug.

re @Fabianx

I also think avoiding the early flush should be the default for registered shutdown functions with an option to opt-in to have this shutdown function be a "true" background callback.

Ok I changed the default to TRUE for $prevent_early_flush parameter.
So now the only way preventEarlyFlush() would return TRUE now is if no functions were registered or if only functions were registered that explicitly set $prevent_early_flush to FALSE. If any 1 function is registered with $prevent_early_flush TRUE OR without a value for $prevent_early_flush then preventEarlyFlush() would return FALSE.

Also renamed the class from ShutDownRegistry from ShutdownRegistry(caps change) b/c shutdown is 1 word.

Deprecated drupal_register_shutdown_function() and replaced logic with calls to the new service.

Updated _drupal_shutdown_function() to call \Drupal\Core\Utility\ShutdownRegistry::getCallbacks()

Also I had misread drupal_register_shutdown_function() specifically

* @param ...
 *   Additional arguments to pass to the shutdown function.

Updated the function signature to
public function registerShutdownCallable(callable $callable, $arguments = [], $prevent_early_flush = TRUE)
$arguments is new here.

So now only batch.inc is using registerShutdownCallable(). I haven't replaced other uses to see if we get any errors. Calls to drupal_register_shutdown_function() will still work but will not be able to set $prevent_early_flush.

I think I missed @catch's point

but if this is a critical bug, let's fix the bug, not add new features based on the bug.

It seems like simplest way to do this would be to extend Response and check drupal_register_shutdown_function() directly to see if any functions were registered.

Here is patch that does that.

Not doing an interdiff because missing most changes from previous patch.

Whoops uploaded the patch 2x

That looks like a good compromise to me, thanks!

That is not quite enough as in the JS-version of the Batch _batch_do() returns a JsonResponse directly which still goes through the upstream logic in Symfony with #16. So we need a specialized JsonRepsonse as well.

1. +++ a/core/lib/Drupal/Core/Render/ResponseTrait.php
   @@ -0,0 +1,48 @@
   + * Determines whether fastcgi_finish_request should be called depending on
   + * based on whether any shutdown functions are registered.
   

   "depending on based on" -> one of both should be removed :).

2. +++ a/core/lib/Drupal/Core/Render/ResponseTrait.php
   @@ -0,0 +1,48 @@
   +   * @return Response
   

   I think this should be return $this? Or why are we not using just inheritdoc?

3. +++ a/core/lib/Drupal/Core/Render/ResponseTrait.php
   @@ -0,0 +1,48 @@
   +   *   True if any shutdown functions are registered.
   

   True -> TRUE and also "otherwise" is missing.

Also in the doc of the trait or the send method I would like to see at least one sentence giving the batch processing as an example why this is needed :).

Discussed with @catch, @cilefen, @cottser, @laurii, and @xjm and we agreed that this was critical because of data integrity. There is a chance that a user delete batch might get halfway through and leave the site in an inconsistent state - with only some of a users content deleted or changed to anonymous. Also many site owners don't choose whether they are using fastcgi so can't fix it by making different choices.

I think we need a CR to tell people about the new trait and the issue if you extend a Symfony response.

- Create a service to register shutdown functions, which runs after sending the response, but before shutting the connection down.
- Convert batchs usage of register_shutdown_function to this new service

Can the issue summary try to explain why this wasn't explored? I guess one disadvantage would be that the current shutdown function solution even works in the case of running into php timeout limits?

I'm really curious why noone has filed an upstream bug: https://github.com/symfony/symfony/pull/21959

Thanks @alexpott and @dawehner. It took me enough time to debug through this that I didn't really bother to consider the larger implications of this.

I tend to agree with the first part of #23 (and I guess some earlier comments, but didn't read the entire issue, sorry) that Batch API really is misusing the shutdown function system. If it is doing stuff in a shutdown function that depends on the subsequent request not having been started that just feels wrong.

If we do go with the current approach, then we should really hope that the Symfony PR goes in, because otherwise we would have IMO to (in a follow-up) create subclasses for all Symfony Response classes and replace all usages of them within Drupal. Otherwise we're just kicking the can down the road, i.e. if someone somehow uses any other type of Response with the Batch system they will hit this same very cryptic and hard to debug problem.

How do others feel?

I tend to agree with the first part of #23 (and I guess some earlier comments, but didn't read the entire issue, sorry) that Batch API really is misusing the shutdown function system. If it is doing stuff in a shutdown function that depends on the subsequent request not having been started that just feels wrong.

Well, there has to be a reason why its not simply doing stuff on the end of its controller function.

If we do go with the current approach, then we should really hope that the Symfony PR goes in, because otherwise we would have IMO to (in a follow-up) create subclasses for all Symfony Response classes and replace all usages of them within Drupal. Otherwise we're just kicking the can down the road, i.e. if someone somehow uses any other type of Response with the Batch system they will hit this same very cryptic and hard to debug problem.

Yeah the subclasses feel quite hacky.

@tstoeckler
Do you mind commenting on the symfony issue with a more detailed reason why we kinda need that?

Batch could possibly continue to register the shutdown function, but also implement a late request listener which does the same thing, and sets something to prevent the shutdown from running (or make it re-entrant and just let it run twice).

The reason it's using shutdown is so it runs even if there's a fatal error, a late request listener is not going to help with that at all. However when there is such an error, there's not going to be a successful subsequent batch request, so it might be OK like that.

I tried to experiment with the result of the latest discussion.

Hmm... why do we actually need a response subscriber when we know exactly for which responses we need this behavior?

Here's something I cooked up in the flight back from Seville, so I think the tag still counts ;-)

Couple of nits on the comments, but that looks exactly like #26, thanks!

1. +++ b/core/includes/batch.inc
   @@ -47,8 +47,21 @@ function _batch_page(Request $request) {
   +  // to handle this case. Because with FastCGI shutdown functions are called
   

   FastCGI and fastcgi_finish_request()

2. +++ b/core/includes/batch.inc
   @@ -47,8 +47,21 @@ function _batch_page(Request $request) {
   +  // shutdown functions would lead to race conditions between consecutive
   

   s/functions/function/

Without significantly changing batch itself (or at least its error handling), while it's not the nicest patch it feels OK. We should possibly open a follow-up to discuss wider alternatives.

Made changes as per the comment #30. Applying the patch, please review.

Nice! I really like that. Its a small change to fix the problem.

Oh, I had a little kernel test for the new function lying around, apparently that didn't make it into the patch... #fail

+++ b/core/includes/batch.inc
@@ -509,7 +551,7 @@ function _batch_finished() {
+  if ($batch = batch_get() && _batch_needs_update()) {

I just realized this does not do what I wanted it to do: It sets $batch to a boolean instead of fetching the batch and checking the function, due to missing dependenciesparentheses.

Not sure, but I think this cannot actually be tested due to the nature of the Batch API and the necessity to trigger a fatal.

Edit: Brain fail -> wrong word

Not sure, but I think this cannot actually be tested due to the nature of the Batch API and the necessity to trigger a fatal.

If this help, I have never been able to run an upgrade test in my current environment without this patch, this for sure make the batch system work at least with php-fpm.

Updated the patch to not make $batch a Boolean.

We had problems with race conditions when large views objects get serialized into the batch table. We started to write a patch which looked pretty much the same as #38. We are using #38 now and it works perfectly.

I also fired manually a fatal error in my batch. The update function in _batch_shutdown() gets fired as it is supposed to.

I've updated the issue summary to include the solution in the patch.

I can't think of a good way to test this patch as a whole, so happy with the RTBC here, sent the patch for a re-test against 8.5.x though.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

No change record needed because there's no API change with the approach we went with.

Tagging for inclusion in 8.4.0-beta1 release notes.

Can someone help with a two line summary for #2895685: Create release notes (a.k.a CHANGELOG) for Drupal 8.4.0-alpha1 and Drupal 8.4.0? After staring at this issue for quite a while, I would not say I can intelligently summarize the problem/solution, sorry :/

I propose:
[Batch processes are broken due to a race condition when using fastcgi.](https://www.drupal.org/node/2851111)

Well, they are not anymore ;) We are supposed to list what were problems before and how we solved them. See other entries in #2895685: Create release notes (a.k.a CHANGELOG) for Drupal 8.4.0-alpha1 and Drupal 8.4.0. I managed to gather that there was something broken with fastcgi and batches. The idea with the release notes is to give some useful summary instead of just an index of issues.

Ok, then how about that?

A race condition occured in the Batch API when using fastcgi. The Batch API now ensures that (the current batch state is written completely to the database before starting the next batch)[https://www.drupal.org/node/2851111].

Superb, exactly what we needed, thanks!