From 811a53ce25922ac83000e9c5dfabe2d78293e7c2 Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Mon, 2 Feb 2026 11:43:39 +0100 Subject: [PATCH 1/2] perf(response): Flush requests early to clients If not, the response will wait for async actions, e.g. HTTP requests from IClientService, to be finished before returning. This is not desired in many cases with e.g. with notifications. Signed-off-by: Joas Schilling --- lib/private/AppFramework/App.php | 8 ++++++++ lib/public/AppFramework/Http/Response.php | 22 ++++++++++++++++++++++ 2 files changed, 30 insertions(+) diff --git a/lib/private/AppFramework/App.php b/lib/private/AppFramework/App.php index 0503f309ac054..a5c0f98d5706e 100644 --- a/lib/private/AppFramework/App.php +++ b/lib/private/AppFramework/App.php @@ -198,5 +198,13 @@ public static function main( $io->setOutput($output); } } + + if ($response->getFlushEarly()) { + fastcgi_finish_request(); + while (ob_get_level() > 0) { + ob_end_flush(); + } + flush(); + } } } diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index cccd6a318a4c7..186889481b28e 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -64,6 +64,7 @@ class Response { /** @var bool */ private $throttled = false; + private bool $flushEarly = true; /** @var array */ private $throttleMetadata = []; @@ -397,4 +398,25 @@ public function getThrottleMetadata() { public function isThrottled() { return $this->throttled; } + + /** + * Request the response should be flushed to the connected client immediately + * + * @since 34.0.0 + */ + public function setFlushEarly(bool $flushEarly): void { + $this->flushEarly = $flushEarly; + } + + /** + * Whether the response should be flushed to the connected client immediately + * + * If not, the response will wait for async actions, e.g. HTTP requests from + * IClientService, to be finished before returning. + * + * @since 34.0.0 + */ + public function getFlushEarly(): bool { + return $this->flushEarly; + } } From 677634f69072549f933007bf212448d2ed3f714c Mon Sep 17 00:00:00 2001 From: Joas Schilling Date: Fri, 31 Jul 2026 15:54:50 +0200 Subject: [PATCH 2/2] perf(response): Handle different SAPIs better and document implications Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Joas Schilling --- build/stubs/sapi.php | 16 ++++ lib/private/AppFramework/App.php | 40 +++++++-- lib/private/AppFramework/Http/Output.php | 25 ++++++ lib/public/AppFramework/Http/IOutput.php | 10 +++ lib/public/AppFramework/Http/Response.php | 28 ++++-- psalm.xml | 1 + tests/lib/AppFramework/AppTest.php | 95 +++++++++++++++++++- tests/lib/AppFramework/Http/OutputTest.php | 49 ++++++++++ tests/lib/AppFramework/Http/ResponseTest.php | 12 +++ 9 files changed, 262 insertions(+), 14 deletions(-) create mode 100644 build/stubs/sapi.php diff --git a/build/stubs/sapi.php b/build/stubs/sapi.php new file mode 100644 index 0000000000000..cf5ede4f0398b --- /dev/null +++ b/build/stubs/sapi.php @@ -0,0 +1,16 @@ +getFlushEarly()) { - fastcgi_finish_request(); - while (ob_get_level() > 0) { - ob_end_flush(); + if ($response->getFlushEarly() && $container->get(IConfig::class)->getSystemValueBool('flush_response_early', true)) { + self::finishRequest($container, $io); + } + } + + /** + * Hand the finished response to the client, so that the remaining work of + * this process does not keep it waiting. + */ + private static function finishRequest(DIContainer $container, IOutput $io): void { + // The remaining work must not be cut off when the client goes away + ignore_user_abort(true); + + // Otherwise the next request of the same client blocks on the session + // lock for as long as this process keeps working + try { + $container->get(ISession::class)->close(); + } catch (ContainerExceptionInterface) { + } + + // The client may send a follow-up request that then reads data from + // before the transaction + try { + if ($container->get(IDBConnection::class)->inTransaction()) { + $container->get(LoggerInterface::class)->warning( + 'A response was flushed to the client while a database transaction was still open. The client may not be able to observe the pending writes yet.', + ['app' => 'core'], + ); } - flush(); + } catch (ContainerExceptionInterface) { } + + $io->finishRequest(); } } diff --git a/lib/private/AppFramework/Http/Output.php b/lib/private/AppFramework/Http/Output.php index 83b6ee6e45089..038d7d1d360be 100644 --- a/lib/private/AppFramework/Http/Output.php +++ b/lib/private/AppFramework/Http/Output.php @@ -88,4 +88,29 @@ public function setCookie($name, $value, $expire, $path, $domain, $secure, $http 'samesite' => $sameSite ]); } + + #[\Override] + public function finishRequest(): bool { + // php-fpm, and the SAPIs aliasing it (recent LiteSpeed, FrankenPHP) + if (function_exists('fastcgi_finish_request')) { + fastcgi_finish_request(); + return true; + } + + if (function_exists('litespeed_finish_request')) { + litespeed_finish_request(); + return true; + } + + // mod_php, cgi, cli, … cannot give the connection back to the web server, + // so only push out what we have. ob_flush() instead of ob_end_flush() keeps + // the buffer around for error handling, and is silenced because output + // handlers are allowed to be non-flushable. + if (ob_get_level() > 0) { + @ob_flush(); + } + flush(); + + return false; + } } diff --git a/lib/public/AppFramework/Http/IOutput.php b/lib/public/AppFramework/Http/IOutput.php index bd20251a8ba2e..753617b87df96 100644 --- a/lib/public/AppFramework/Http/IOutput.php +++ b/lib/public/AppFramework/Http/IOutput.php @@ -57,4 +57,14 @@ public function setHttpResponseCode($code); * @since 8.1.0 */ public function setCookie($name, $value, $expire, $path, $domain, $secure, $httpOnly, $sameSite = 'Lax'); + + /** + * Send the response produced so far to the client, so the remaining work of + * this process does not delay it. Nothing may be written to the output after. + * + * @return bool true if the connection was closed, false if the response was + * only flushed because the SAPI does not support closing it + * @since 35.0.0 + */ + public function finishRequest(): bool; } diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php index 186889481b28e..623879d5abba8 100644 --- a/lib/public/AppFramework/Http/Response.php +++ b/lib/public/AppFramework/Http/Response.php @@ -64,7 +64,7 @@ class Response { /** @var bool */ private $throttled = false; - private bool $flushEarly = true; + private bool $flushEarly = false; /** @var array */ private $throttleMetadata = []; @@ -400,21 +400,33 @@ public function isThrottled() { } /** - * Request the response should be flushed to the connected client immediately + * Request that the response is sent to the client as soon as it is complete, + * instead of when the PHP process is done. Enable this only when the + * controller leaves work behind that the client does not have to wait for. * - * @since 34.0.0 + * Things to be aware of before enabling this: + * + * - The session is closed before the response goes out, so anything running + * afterwards can no longer write to it. + * - The status code and the body are final at that point. Errors happening + * afterwards can only be logged, never reported to the client. + * - Nothing may be written to the output afterwards, the Content-Length has + * already been announced. + * - The worker of the web server stays occupied until the process really + * ends, so this improves latency but not throughput. + * - Administrators can turn this off globally with the `flush_response_early` + * system config option + * + * @since 35.0.0 */ public function setFlushEarly(bool $flushEarly): void { $this->flushEarly = $flushEarly; } /** - * Whether the response should be flushed to the connected client immediately - * - * If not, the response will wait for async actions, e.g. HTTP requests from - * IClientService, to be finished before returning. + * Whether the response should be sent to the client as soon as it is complete * - * @since 34.0.0 + * @since 35.0.0 */ public function getFlushEarly(): bool { return $this->flushEarly; diff --git a/psalm.xml b/psalm.xml index 6e04adfaade86..7b1d967ddbb7c 100644 --- a/psalm.xml +++ b/psalm.xml @@ -129,6 +129,7 @@ + diff --git a/tests/lib/AppFramework/AppTest.php b/tests/lib/AppFramework/AppTest.php index 334eeefdab6c2..ba60ca6dc3012 100644 --- a/tests/lib/AppFramework/AppTest.php +++ b/tests/lib/AppFramework/AppTest.php @@ -14,6 +14,11 @@ use OCP\AppFramework\Controller; use OCP\AppFramework\Http\IOutput; use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Http\StreamResponse; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\ISession; +use Psr\Log\LoggerInterface; function rrmdir($directory) { $files = array_diff(scandir($directory), ['.','..']); @@ -86,6 +91,92 @@ public function testControllerNameAndMethodAreBeingPassed(): void { $this->container); } + public function testRequestIsNotFinishedEarlyByDefault(): void { + $this->dispatcher->expects($this->once()) + ->method('dispatch') + ->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, new Response()]); + + $this->io->expects($this->never()) + ->method('finishRequest'); + + App::main($this->controllerName, $this->controllerMethod, $this->container, []); + } + + public function testRequestIsFinishedEarlyWhenTheResponseAsksForIt(): void { + $response = new Response(); + $response->setFlushEarly(true); + + $this->dispatcher->expects($this->once()) + ->method('dispatch') + ->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]); + + $session = $this->createMock(ISession::class); + $session->expects($this->once()) + ->method('close'); + $this->container[ISession::class] = $session; + + $connection = $this->createMock(IDBConnection::class); + $connection->method('inTransaction') + ->willReturn(false); + $this->container[IDBConnection::class] = $connection; + + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->never()) + ->method('warning'); + $this->container[LoggerInterface::class] = $logger; + + $this->io->expects($this->once()) + ->method('finishRequest'); + + App::main($this->controllerName, $this->controllerMethod, $this->container, []); + } + + public function testRequestIsNotFinishedEarlyWhenDisabledByTheAdmin(): void { + $response = new Response(); + $response->setFlushEarly(true); + + $this->dispatcher->expects($this->once()) + ->method('dispatch') + ->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]); + + $config = $this->createMock(IConfig::class); + $config->method('getSystemValueBool') + ->with('flush_response_early', true) + ->willReturn(false); + $this->container[IConfig::class] = $config; + + $this->io->expects($this->never()) + ->method('finishRequest'); + + App::main($this->controllerName, $this->controllerMethod, $this->container, []); + } + + public function testOpenTransactionIsLoggedWhenFinishingEarly(): void { + $response = new Response(); + $response->setFlushEarly(true); + + $this->dispatcher->expects($this->once()) + ->method('dispatch') + ->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]); + + $this->container[ISession::class] = $this->createMock(ISession::class); + + $connection = $this->createMock(IDBConnection::class); + $connection->method('inTransaction') + ->willReturn(true); + $this->container[IDBConnection::class] = $connection; + + $logger = $this->createMock(LoggerInterface::class); + $logger->expects($this->once()) + ->method('warning'); + $this->container[LoggerInterface::class] = $logger; + + $this->io->expects($this->once()) + ->method('finishRequest'); + + App::main($this->controllerName, $this->controllerMethod, $this->container, []); + } + public function testBuildAppNamespace(): void { $ns = App::buildAppNamespace('someapp'); $this->assertEquals('OCA\Someapp', $ns); @@ -144,7 +235,9 @@ public function testNoOutput(string $statusCode): void { } public function testCallbackIsCalled(): void { - $mock = $this->getMockBuilder('OCP\AppFramework\Http\ICallbackResponse') + // The dispatcher always hands back a Response, not a bare ICallbackResponse + $mock = $this->getMockBuilder(StreamResponse::class) + ->disableOriginalConstructor() ->getMock(); $return = ['HTTP/2.0 200 OK', [], [], $this->output, $mock]; diff --git a/tests/lib/AppFramework/Http/OutputTest.php b/tests/lib/AppFramework/Http/OutputTest.php index 2ba93833dd135..635b3004013ab 100644 --- a/tests/lib/AppFramework/Http/OutputTest.php +++ b/tests/lib/AppFramework/Http/OutputTest.php @@ -27,4 +27,53 @@ public function testSetReadfileStream(): void { $output = new Output(''); $output->setReadfile(fopen(__FILE__, 'r')); } + + /** The buffer has to survive the flush so error handling can still use it */ + public function testFinishRequestKeepsTheOutputBuffer(): void { + $this->skipIfConnectionCanBeClosed(); + $output = new Output(''); + + ob_start(); + $levelBefore = ob_get_level(); + $closed = $output->finishRequest(); + $levelAfter = ob_get_level(); + ob_end_clean(); + + $this->assertFalse($closed); + $this->assertSame($levelBefore, $levelAfter); + } + + /** Draining a handler that refuses to be flushed in a loop would never end */ + public function testFinishRequestWithNonFlushableBuffer(): void { + $this->skipIfConnectionCanBeClosed(); + $output = new Output(''); + + ob_start( + static fn (string $buffer): string => $buffer, + 0, + PHP_OUTPUT_HANDLER_CLEANABLE | PHP_OUTPUT_HANDLER_REMOVABLE, + ); + $levelBefore = ob_get_level(); + $closed = $output->finishRequest(); + $levelAfter = ob_get_level(); + ob_end_clean(); + + $this->assertFalse($closed); + $this->assertSame($levelBefore, $levelAfter); + } + + public function testFinishRequestWithoutAnyOutputBuffer(): void { + $this->skipIfConnectionCanBeClosed(); + $output = new Output(''); + + $level = ob_get_level(); + $this->assertFalse($output->finishRequest()); + $this->assertSame($level, ob_get_level()); + } + + private function skipIfConnectionCanBeClosed(): void { + if (function_exists('fastcgi_finish_request') || function_exists('litespeed_finish_request')) { + $this->markTestSkipped('This SAPI closes the connection, which would tear down the output buffer of the test runner'); + } + } } diff --git a/tests/lib/AppFramework/Http/ResponseTest.php b/tests/lib/AppFramework/Http/ResponseTest.php index 4af5cb2443fcb..b473deb0ac161 100644 --- a/tests/lib/AppFramework/Http/ResponseTest.php +++ b/tests/lib/AppFramework/Http/ResponseTest.php @@ -259,4 +259,16 @@ public function testGetThrottleMetadata(): void { $this->childResponse->throttle(['foo' => 'bar']); $this->assertSame(['foo' => 'bar'], $this->childResponse->getThrottleMetadata()); } + + public function testFlushEarlyIsOptIn(): void { + $this->assertFalse($this->childResponse->getFlushEarly()); + } + + public function testSetFlushEarly(): void { + $this->childResponse->setFlushEarly(true); + $this->assertTrue($this->childResponse->getFlushEarly()); + + $this->childResponse->setFlushEarly(false); + $this->assertFalse($this->childResponse->getFlushEarly()); + } }