From 69bf50fee050a25c0f758aa39a6ef3b7d763c451 Mon Sep 17 00:00:00 2001
From: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date: Fri, 11 Aug 2017 14:50:42 +0900
Subject: [PATCH] Issue #2851111 by tedbow, tstoeckler, Munavijayalakshmi,
 dagmar, dawehner, catch, Fabianx, david_garcia, Nebel54, hchonov:
 fastcgi_finish_request and shutdown functions (e.g. batch) do not play nicely
 together

---
 core/includes/batch.inc                       | 52 +++++++++++++++++--
 .../Core/Batch/BatchKernelTest.php            | 40 ++++++++++++++
 2 files changed, 87 insertions(+), 5 deletions(-)
 create mode 100644 core/tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php

diff --git a/core/includes/batch.inc b/core/includes/batch.inc
index 5d05cd503929..402f195c2f19 100644
--- a/core/includes/batch.inc
+++ b/core/includes/batch.inc
@@ -47,8 +47,22 @@ function _batch_page(Request $request) {
     }
   }
 
-  // Register database update for the end of processing.
+  // We need to store the updated batch information in the batch storage after
+  // processing the batch. In order for the error page to work correctly this
+  // needs to be done even in case of a PHP fatal error in which case the end of
+  // this function is never reached. Therefore we register a shutdown function
+  // to handle this case. Because with FastCGI and fastcgi_finish_request()
+  // shutdown functions are called after the HTTP connection is closed, updating
+  // the batch information in a shutdown function would lead to race conditions
+  // between consecutive requests if the batch processing continues. In case of
+  // a fatal error the processing stops anyway, so it works even with FastCGI.
+  // However, we must ensure to only update in the shutdown phase in this
+  // particular case we track whether the batch information still needs to be
+  // updated.
+  // @see _batch_shutdown()
+  // @see \Symfony\Component\HttpFoundation\Response::send()
   drupal_register_shutdown_function('_batch_shutdown');
+  _batch_needs_update(TRUE);
 
   $build = [];
 
@@ -70,18 +84,46 @@ function _batch_page(Request $request) {
       $current_set = _batch_current_set();
       $build['#title'] = $current_set['title'];
       $build['content'] = _batch_progress_page();
+
+      $response = $build;
       break;
 
     case 'do':
       // JavaScript-based progress page callback.
-      return _batch_do();
+      $response = _batch_do();
+      break;
 
     case 'finished':
       // _batch_finished() returns a RedirectResponse.
-      return _batch_finished();
+      $response = _batch_finished();
+      break;
   }
 
-  return $build;
+  if ($batch) {
+    \Drupal::service('batch.storage')->update($batch);
+  }
+  _batch_needs_update(FALSE);
+
+  return $response;
+}
+
+/**
+ * Checks whether the batch information needs to be updated in the storage.
+ *
+ * @param bool $new_value
+ *   (optional) A new value to set.
+ *
+ * @return bool
+ *   TRUE if the batch information needs to be updated; FALSE otherwise.
+ */
+function _batch_needs_update($new_value = NULL) {
+  $needs_update = &drupal_static(__FUNCTION__, FALSE);
+
+  if (isset($new_value)) {
+    $needs_update = $new_value;
+  }
+
+  return $needs_update;
 }
 
 /**
@@ -509,7 +551,7 @@ function _batch_finished() {
  * @see drupal_register_shutdown_function()
  */
 function _batch_shutdown() {
-  if ($batch = batch_get()) {
+  if (($batch = batch_get()) && _batch_needs_update()) {
     \Drupal::service('batch.storage')->update($batch);
   }
 }
diff --git a/core/tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php b/core/tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php
new file mode 100644
index 000000000000..624030872c28
--- /dev/null
+++ b/core/tests/Drupal/KernelTests/Core/Batch/BatchKernelTest.php
@@ -0,0 +1,40 @@
+<?php
+
+namespace Drupal\KernelTests\Core\Batch;
+
+use Drupal\KernelTests\KernelTestBase;
+
+/**
+ * Tests batch functionality.
+ *
+ * @group Batch
+ */
+class BatchKernelTest extends KernelTestBase {
+
+  /**
+   * {@inheritdoc}
+   */
+  protected function setUp() {
+    parent::setUp();
+
+    require_once $this->root . '/core/includes/batch.inc';
+  }
+
+  /**
+   * Tests _batch_needs_update().
+   */
+  public function testNeedsUpdate() {
+    // Before ever being called, the return value should be FALSE.
+    $this->assertEquals(FALSE, _batch_needs_update());
+
+    // Set the value to TRUE.
+    $this->assertEquals(TRUE, _batch_needs_update(TRUE));
+    // Check that without a parameter TRUE is returned.
+    $this->assertEquals(TRUE, _batch_needs_update());
+
+    // Set the value to FALSE.
+    $this->assertEquals(FALSE, _batch_needs_update(FALSE));
+    $this->assertEquals(FALSE, _batch_needs_update());
+  }
+
+}
-- 
GitLab