From c4690891f3649720c7faaaa31a0849af7632bc2b Mon Sep 17 00:00:00 2001
From: catch <catch@35733.no-reply.drupal.org>
Date: Tue, 18 Jul 2023 13:47:42 +0100
Subject: [PATCH] Issue #3295790 by bradjones1, mstrelan, rpayanm, catch,
alexpott, acbramley, longwave, kim.pepper, mfb: Post-response task running
(destructable services) are actually blocking; add test coverage and warn for
common misconfiguration
---
.../FinishResponseSubscriber.php | 20 +++++++++
core/misc/cspell/dictionary.txt | 1 +
.../HtmlResponseBigPipeSubscriber.php | 6 +++
.../src/Functional/JsonApiRegressionTest.php | 18 +++++---
.../jsonapi/tests/src/Functional/NodeTest.php | 4 ++
.../ConfigurableLanguageManagerTest.php | 12 +++++
.../src/Functional/LocaleLocaleLookupTest.php | 5 +++
.../tests/src/Functional/PathAliasTest.php | 4 ++
core/modules/system/system.install | 11 +++++
.../destructable_test.info.yml | 5 +++
.../destructable_test.routing.yml | 7 +++
.../destructable_test.services.yml | 4 ++
.../CallsDestructableServiceController.php | 45 +++++++++++++++++++
.../destructable_test/src/Destructable.php | 34 ++++++++++++++
.../src/Controller/SystemTestController.php | 4 +-
.../System/ShutdownFunctionsTest.php | 10 ++---
.../HttpKernel/DestructableServiceTest.php | 43 ++++++++++++++++++
core/tests/Drupal/Tests/ApiRequestTrait.php | 6 +++
18 files changed, 225 insertions(+), 14 deletions(-)
create mode 100644 core/modules/system/tests/modules/destructable_test/destructable_test.info.yml
create mode 100644 core/modules/system/tests/modules/destructable_test/destructable_test.routing.yml
create mode 100644 core/modules/system/tests/modules/destructable_test/destructable_test.services.yml
create mode 100644 core/modules/system/tests/modules/destructable_test/src/Controller/CallsDestructableServiceController.php
create mode 100644 core/modules/system/tests/modules/destructable_test/src/Destructable.php
create mode 100644 core/tests/Drupal/FunctionalTests/HttpKernel/DestructableServiceTest.php
diff --git a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
index b5fc2aaf25a6..8dc9bac392ea 100644
--- a/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
+++ b/core/lib/Drupal/Core/EventSubscriber/FinishResponseSubscriber.php
@@ -12,6 +12,7 @@
use Drupal\Core\Site\Settings;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
+use Symfony\Component\HttpFoundation\StreamedResponse;
use Symfony\Component\HttpKernel\Event\ResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
@@ -298,6 +299,21 @@ protected function setExpiresNoCache(Response $response) {
$response->setExpires(\DateTime::createFromFormat('j-M-Y H:i:s T', '19-Nov-1978 05:00:00 UTC'));
}
+ /**
+ * Sets the Content-Length header on the response.
+ *
+ * @param \Symfony\Component\HttpKernel\Event\ResponseEvent $event
+ * The event to process.
+ */
+ public function setContentLengthHeader(ResponseEvent $event): void {
+ $response = $event->getResponse();
+ if ($response instanceof StreamedResponse) {
+ return;
+ }
+
+ $response->headers->set('Content-Length', strlen($response->getContent()), TRUE);
+ }
+
/**
* Registers the methods in this class that should be listeners.
*
@@ -309,6 +325,10 @@ public static function getSubscribedEvents(): array {
// There is no specific reason for choosing 16 beside it should be executed
// before ::onRespond().
$events[KernelEvents::RESPONSE][] = ['onAllResponds', 16];
+ // Run very late, after all other response subscribers have run. However,
+ // any response subscribers that convert a response to a streamed response
+ // must run after this and undo what this does.
+ $events[KernelEvents::RESPONSE][] = ['setContentLengthHeader', -1024];
return $events;
}
diff --git a/core/misc/cspell/dictionary.txt b/core/misc/cspell/dictionary.txt
index 1719f779143c..0be92d6ffde0 100644
--- a/core/misc/cspell/dictionary.txt
+++ b/core/misc/cspell/dictionary.txt
@@ -625,6 +625,7 @@ linkset
linktext
lisu
litererally
+litespeed
llamaids
llamasarelame
llamma
diff --git a/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
index e818522a69ff..03e1d4261cd9 100644
--- a/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
+++ b/core/modules/big_pipe/src/EventSubscriber/HtmlResponseBigPipeSubscriber.php
@@ -82,6 +82,8 @@ public function onRespond(ResponseEvent $event) {
$content = $response->getContent();
$content = str_replace('<drupal-big-pipe-scripts-bottom-marker>', '', $content);
$response->setContent($content);
+ // FinishResponseSubscriber::setContentLengthHeader() already ran.
+ $response->headers->set('Content-Length', strlen($content), TRUE);
}
// If there are neither BigPipe placeholders nor no-JS BigPipe placeholders,
@@ -93,6 +95,10 @@ public function onRespond(ResponseEvent $event) {
$big_pipe_response = new BigPipeResponse($response);
$big_pipe_response->setBigPipeService($this->getBigPipeService($event));
+
+ // A BigPipe response's length is impossible to predict.
+ $big_pipe_response->headers->remove('Content-Length');
+
$event->setResponse($big_pipe_response);
}
diff --git a/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
index f88fa41416d0..b3cc1c0df83d 100644
--- a/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/JsonApiRegressionTest.php
@@ -1300,10 +1300,13 @@ public function testNonCacheableMethods() {
$methods = [
'HEAD',
'GET',
- 'PATCH',
- 'DELETE',
];
- $non_post_request_options = $base_request_options + [
+ foreach ($methods as $method) {
+ $response = $this->request($method, Url::fromUri('internal:/jsonapi/node/article/' . $node->uuid()), $base_request_options);
+ $this->assertSame(200, $response->getStatusCode());
+ }
+
+ $patch_request_options = $base_request_options + [
RequestOptions::JSON => [
'data' => [
'type' => 'node--article',
@@ -1311,10 +1314,11 @@ public function testNonCacheableMethods() {
],
],
];
- foreach ($methods as $method) {
- $response = $this->request($method, Url::fromUri('internal:/jsonapi/node/article/' . $node->uuid()), $non_post_request_options);
- $this->assertSame($method === 'DELETE' ? 204 : 200, $response->getStatusCode());
- }
+ $response = $this->request('PATCH', Url::fromUri('internal:/jsonapi/node/article/' . $node->uuid()), $patch_request_options);
+ $this->assertSame(200, $response->getStatusCode());
+
+ $response = $this->request('DELETE', Url::fromUri('internal:/jsonapi/node/article/' . $node->uuid()), $base_request_options);
+ $this->assertSame(204, $response->getStatusCode());
$post_request_options = $base_request_options + [
RequestOptions::JSON => [
diff --git a/core/modules/jsonapi/tests/src/Functional/NodeTest.php b/core/modules/jsonapi/tests/src/Functional/NodeTest.php
index 9c9f5b16c607..d0893201e1a8 100644
--- a/core/modules/jsonapi/tests/src/Functional/NodeTest.php
+++ b/core/modules/jsonapi/tests/src/Functional/NodeTest.php
@@ -393,11 +393,15 @@ protected function assertCacheableNormalizations(): void {
$request_options = $this->getAuthenticationRequestOptions();
$request_options[RequestOptions::QUERY] = ['fields' => ['node--camelids' => 'title']];
$this->request('GET', $url, $request_options);
+ // Cacheable normalizations are written after the response is flushed to
+ // the client; give the server a chance to complete this work.
+ sleep(1);
// Ensure the normalization cache is being incrementally built. After
// requesting the title, only the title is in the cache.
$this->assertNormalizedFieldsAreCached(['title']);
$request_options[RequestOptions::QUERY] = ['fields' => ['node--camelids' => 'field_rest_test']];
$this->request('GET', $url, $request_options);
+ sleep(1);
// After requesting an additional field, then that field is in the cache and
// the old one is still there.
$this->assertNormalizedFieldsAreCached(['title', 'field_rest_test']);
diff --git a/core/modules/language/tests/src/Functional/ConfigurableLanguageManagerTest.php b/core/modules/language/tests/src/Functional/ConfigurableLanguageManagerTest.php
index d6b71e3a36a5..2bd7360721c7 100644
--- a/core/modules/language/tests/src/Functional/ConfigurableLanguageManagerTest.php
+++ b/core/modules/language/tests/src/Functional/ConfigurableLanguageManagerTest.php
@@ -266,4 +266,16 @@ public function testUserProfileTranslationWithPreferredAdminLanguage() {
$assert_session->pageTextNotContains($field_label_es);
}
+ /**
+ * {@inheritdoc}
+ */
+ protected function drupalGet($path, array $options = [], array $headers = []) {
+ $response = parent::drupalGet($path, $options, $headers);
+ // The \Drupal\locale\LocaleTranslation service clears caches after the
+ // response is flushed to the client; wait for Drupal to perform its
+ // termination work before continuing.
+ sleep(1);
+ return $response;
+ }
+
}
diff --git a/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
index 9ba855d1f158..5b43ad50bf1a 100644
--- a/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
+++ b/core/modules/locale/tests/src/Functional/LocaleLocaleLookupTest.php
@@ -68,6 +68,11 @@ public function testLanguageFallbackDefaults() {
* @dataProvider providerTestFixOldPluralStyle
*/
public function testFixOldPluralStyle($translation_value, $expected) {
+ // The \Drupal\locale\LocaleTranslation service stores localization cache
+ // data after the response is flushed to the client. We do not want to race
+ // with any string translations that may be saving from the login in
+ // ::setUp().
+ sleep(1);
$string_storage = \Drupal::service('locale.storage');
$string = $string_storage->findString(['source' => 'Member for', 'context' => '']);
$lid = $string->getId();
diff --git a/core/modules/path/tests/src/Functional/PathAliasTest.php b/core/modules/path/tests/src/Functional/PathAliasTest.php
index 1c8873344bee..f6ebeb9be328 100644
--- a/core/modules/path/tests/src/Functional/PathAliasTest.php
+++ b/core/modules/path/tests/src/Functional/PathAliasTest.php
@@ -66,6 +66,9 @@ public function testPathCache() {
\Drupal::cache('data')->deleteAll();
// Make sure the path is not converted to the alias.
$this->drupalGet(trim($edit['path[0][value]'], '/'), ['alias' => TRUE]);
+ // The \Drupal\path_alias\AliasWhitelist service performs cache clears after
+ // Drupal has flushed the response to the client; wait for this to finish.
+ sleep(1);
$this->assertNotEmpty(\Drupal::cache('data')->get('preload-paths:' . $edit['path[0][value]']), 'Cache entry was created.');
// Visit the alias for the node and confirm a cache entry is created.
@@ -73,6 +76,7 @@ public function testPathCache() {
// @todo Remove this once https://www.drupal.org/node/2480077 lands.
Cache::invalidateTags(['rendered']);
$this->drupalGet(trim($edit['alias[0][value]'], '/'));
+ sleep(1);
$this->assertNotEmpty(\Drupal::cache('data')->get('preload-paths:' . $edit['path[0][value]']), 'Cache entry was created.');
}
diff --git a/core/modules/system/system.install b/core/modules/system/system.install
index cbab5df838eb..c5425e95cf48 100644
--- a/core/modules/system/system.install
+++ b/core/modules/system/system.install
@@ -501,6 +501,17 @@ function system_requirements($phase) {
}
}
+ if ($phase === 'runtime') {
+ if (!function_exists('fastcgi_finish_request') && !function_exists('litespeed_finish_request') && !ob_get_status()) {
+ $requirements['output_buffering'] = [
+ 'title' => t('Output Buffering'),
+ 'error_value' => t('Not enabled'),
+ 'severity' => REQUIREMENT_WARNING,
+ 'description' => t('<a href="https://www.php.net/manual/en/function.ob-start.php">Output buffering</a> is not enabled. This may degrade Drupal\'s performance. You can enable output buffering by default <a href="https://www.php.net/manual/en/outcontrol.configuration.php#ini.output-buffering">in your PHP settings</a>.'),
+ ];
+ }
+ }
+
if ($phase == 'install' || $phase == 'update') {
// Test for PDO (database).
$requirements['database_extensions'] = [
diff --git a/core/modules/system/tests/modules/destructable_test/destructable_test.info.yml b/core/modules/system/tests/modules/destructable_test/destructable_test.info.yml
new file mode 100644
index 000000000000..ed91a2439874
--- /dev/null
+++ b/core/modules/system/tests/modules/destructable_test/destructable_test.info.yml
@@ -0,0 +1,5 @@
+name: 'Tests for DestructableInterface'
+type: module
+description: 'Provides test services which implement DestructableInterface.'
+package: Testing
+version: VERSION
diff --git a/core/modules/system/tests/modules/destructable_test/destructable_test.routing.yml b/core/modules/system/tests/modules/destructable_test/destructable_test.routing.yml
new file mode 100644
index 000000000000..51eaae2d4b7d
--- /dev/null
+++ b/core/modules/system/tests/modules/destructable_test/destructable_test.routing.yml
@@ -0,0 +1,7 @@
+destructable:
+ path: '/destructable'
+ defaults:
+ _controller: '\Drupal\destructable_test\Controller\CallsDestructableServiceController::render'
+ _title: 'Calls destructable service'
+ requirements:
+ _access: 'TRUE'
diff --git a/core/modules/system/tests/modules/destructable_test/destructable_test.services.yml b/core/modules/system/tests/modules/destructable_test/destructable_test.services.yml
new file mode 100644
index 000000000000..316ce275d04d
--- /dev/null
+++ b/core/modules/system/tests/modules/destructable_test/destructable_test.services.yml
@@ -0,0 +1,4 @@
+services:
+ Drupal\destructable_test\Destructable:
+ tags:
+ - { name: needs_destruction }
diff --git a/core/modules/system/tests/modules/destructable_test/src/Controller/CallsDestructableServiceController.php b/core/modules/system/tests/modules/destructable_test/src/Controller/CallsDestructableServiceController.php
new file mode 100644
index 000000000000..42b1e25def8a
--- /dev/null
+++ b/core/modules/system/tests/modules/destructable_test/src/Controller/CallsDestructableServiceController.php
@@ -0,0 +1,45 @@
+<?php
+
+namespace Drupal\destructable_test\Controller;
+
+use Drupal\Core\Controller\ControllerBase;
+use Drupal\destructable_test\Destructable;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Symfony\Component\HttpFoundation\Request;
+use Symfony\Component\HttpFoundation\Response;
+
+/**
+ * Controller to instantiate the destructable service.
+ */
+final class CallsDestructableServiceController extends ControllerBase {
+
+ /**
+ * Destructable service.
+ *
+ * @var \Drupal\destructable_test\Destructable
+ */
+ protected $destructable;
+
+ /**
+ * {@inheritdoc}
+ */
+ public static function create(ContainerInterface $container) {
+ return new static($container->get(Destructable::class));
+ }
+
+ public function __construct(Destructable $destructable) {
+ $this->destructable = $destructable;
+ }
+
+ /**
+ * Render callback.
+ *
+ * @return \Symfony\Component\HttpFoundation\Response
+ * Response.
+ */
+ public function render(Request $request): Response {
+ $this->destructable->setSemaphore($request->query->get('semaphore'));
+ return new Response('This is a longer-ish string of content to send to the client, to invoke any trivial transfer buffers both on the server and client side.');
+ }
+
+}
diff --git a/core/modules/system/tests/modules/destructable_test/src/Destructable.php b/core/modules/system/tests/modules/destructable_test/src/Destructable.php
new file mode 100644
index 000000000000..210adc70aca0
--- /dev/null
+++ b/core/modules/system/tests/modules/destructable_test/src/Destructable.php
@@ -0,0 +1,34 @@
+<?php
+
+namespace Drupal\destructable_test;
+
+use Drupal\Core\DestructableInterface;
+
+final class Destructable implements DestructableInterface {
+
+ /**
+ * Semaphore filename.
+ *
+ * @var string
+ */
+ protected string $semaphore;
+
+ /**
+ * Set the destination for the semaphore file.
+ *
+ * @param string $semaphore
+ * Temporary file to set a semaphore flag.
+ */
+ public function setSemaphore(string $semaphore): void {
+ $this->semaphore = $semaphore;
+ }
+
+ /**
+ * {@inheritdoc}
+ */
+ public function destruct() {
+ sleep(3);
+ file_put_contents($this->semaphore, 'ran');
+ }
+
+}
diff --git a/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
index 03f44fec4ba4..b806e582d6a4 100644
--- a/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
+++ b/core/modules/system/tests/modules/system_test/src/Controller/SystemTestController.php
@@ -332,8 +332,8 @@ public function shutdownFunctions($arg1, $arg2) {
// the exception message can not be tested.
// @see _drupal_shutdown_function()
// @see \Drupal\system\Tests\System\ShutdownFunctionsTest
- if (function_exists('fastcgi_finish_request')) {
- return ['#markup' => 'The function fastcgi_finish_request exists when serving the request.'];
+ if (function_exists('fastcgi_finish_request') || ob_get_status()) {
+ return ['#markup' => 'The response will flush before shutdown functions are called.'];
}
return [];
}
diff --git a/core/modules/system/tests/src/Functional/System/ShutdownFunctionsTest.php b/core/modules/system/tests/src/Functional/System/ShutdownFunctionsTest.php
index 6d7798721c71..7c36abd38160 100644
--- a/core/modules/system/tests/src/Functional/System/ShutdownFunctionsTest.php
+++ b/core/modules/system/tests/src/Functional/System/ShutdownFunctionsTest.php
@@ -42,18 +42,18 @@ public function testShutdownFunctions() {
$arg2 = $this->randomMachineName();
$this->drupalGet('system-test/shutdown-functions/' . $arg1 . '/' . $arg2);
- // If using PHP-FPM then fastcgi_finish_request() will have been fired
- // returning the response before shutdown functions have fired.
+ // If using PHP-FPM or output buffering, the response will be flushed to
+ // the client before shutdown functions have fired.
// @see \Drupal\system_test\Controller\SystemTestController::shutdownFunctions()
- $server_using_fastcgi = strpos($this->getSession()->getPage()->getContent(), 'The function fastcgi_finish_request exists when serving the request.');
- if ($server_using_fastcgi) {
+ $response_will_flush = strpos($this->getSession()->getPage()->getContent(), 'The response will flush before shutdown functions are called.');
+ if ($response_will_flush) {
// We need to wait to ensure that the shutdown functions have fired.
sleep(1);
}
$this->assertEquals([$arg1, $arg2], \Drupal::state()->get('_system_test_first_shutdown_function'));
$this->assertEquals([$arg1, $arg2], \Drupal::state()->get('_system_test_second_shutdown_function'));
- if (!$server_using_fastcgi) {
+ if (!$response_will_flush) {
// Make sure exceptions displayed through
// \Drupal\Core\Utility\Error::renderExceptionSafe() are correctly
// escaped.
diff --git a/core/tests/Drupal/FunctionalTests/HttpKernel/DestructableServiceTest.php b/core/tests/Drupal/FunctionalTests/HttpKernel/DestructableServiceTest.php
new file mode 100644
index 000000000000..38cad0a63854
--- /dev/null
+++ b/core/tests/Drupal/FunctionalTests/HttpKernel/DestructableServiceTest.php
@@ -0,0 +1,43 @@
+<?php
+
+namespace Drupal\FunctionalTests\HttpKernel;
+
+use Drupal\Core\File\FileSystemInterface;
+use Drupal\Core\Url;
+use Drupal\Tests\BrowserTestBase;
+
+/**
+ * Tests invocation of services performing deferred tasks after response flush.
+ *
+ * @see \Drupal\Core\DestructableInterface
+ *
+ * @group Http
+ */
+class DestructableServiceTest extends BrowserTestBase {
+
+ /**
+ * {@inheritdoc}
+ */
+ protected static $modules = ['system', 'destructable_test'];
+
+ /**
+ * {@inheritdoc}
+ */
+ protected $defaultTheme = 'stark';
+
+ public function testDestructableServiceExecutionOrder(): void {
+ $file_system = $this->container->get('file_system');
+ assert($file_system instanceof FileSystemInterface);
+ $semaphore = $file_system
+ ->tempnam($file_system->getTempDirectory(), 'destructable_semaphore');
+ $this->drupalGet(Url::fromRoute('destructable', [], ['query' => ['semaphore' => $semaphore]]));
+ // This should be false as the response should flush before running the
+ // test service.
+ $this->assertEmpty(file_get_contents($semaphore), 'Destructable service did not run when response flushed to client.');
+ // The destructable service will sleep for 3 seconds, then run.
+ // To ensure no race conditions on slow test runners, wait another 3s.
+ sleep(6);
+ $this->assertTrue(file_get_contents($semaphore) === 'ran', 'Destructable service did run.');
+ }
+
+}
diff --git a/core/tests/Drupal/Tests/ApiRequestTrait.php b/core/tests/Drupal/Tests/ApiRequestTrait.php
index 9a5431a2c7e7..83888277415b 100644
--- a/core/tests/Drupal/Tests/ApiRequestTrait.php
+++ b/core/tests/Drupal/Tests/ApiRequestTrait.php
@@ -37,6 +37,12 @@ trait ApiRequestTrait {
* @see \GuzzleHttp\ClientInterface::request()
*/
protected function makeApiRequest($method, Url $url, array $request_options) {
+ // HEAD requests do not have bodies. If one is specified, Guzzle will not
+ // ignore it and the request will be treated as GET with an overridden
+ // method string, and libcurl will expect to read a response body.
+ if ($method === 'HEAD' && array_key_exists('body', $request_options)) {
+ unset($request_options['body']);
+ }
$this->refreshVariables();
$request_options[RequestOptions::HTTP_ERRORS] = FALSE;
$request_options[RequestOptions::ALLOW_REDIRECTS] = FALSE;
--
GitLab