Skip to content

Commit 677634f

Browse files
nickvergessennickvergessen
committed
perf(response): Handle different SAPIs better and document implications
Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Joas Schilling <coding@schilljs.com>
1 parent 811a53c commit 677634f

9 files changed

Lines changed: 262 additions & 14 deletions

File tree

‎build/stubs/sapi.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
<?php
2+
3+
/**
4+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
5+
* SPDX-License-Identifier: AGPL-3.0-or-later
6+
*
7+
* Functions only provided by some SAPIs, always guarded with function_exists().
8+
*/
9+
10+
/** @return bool */
11+
function fastcgi_finish_request() {
12+
}
13+
14+
/** @return void */
15+
function litespeed_finish_request() {
16+
}

‎lib/private/AppFramework/App.php‎

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,14 @@
1919
use OCP\AppFramework\Http\IOutput;
2020
use OCP\Diagnostics\IEventLogger;
2121
use OCP\HintException;
22+
use OCP\IConfig;
23+
use OCP\IDBConnection;
2224
use OCP\IRequest;
25+
use OCP\ISession;
2326
use OCP\Profiler\IProfiler;
2427
use OCP\Server;
2528
use Psr\Container\ContainerExceptionInterface;
29+
use Psr\Log\LoggerInterface;
2630

2731
/**
2832
* Entry point for every request in your app. You can consider this as your
@@ -199,12 +203,38 @@ public static function main(
199203
}
200204
}
201205

202-
if ($response->getFlushEarly()) {
203-
fastcgi_finish_request();
204-
while (ob_get_level() > 0) {
205-
ob_end_flush();
206+
if ($response->getFlushEarly() && $container->get(IConfig::class)->getSystemValueBool('flush_response_early', true)) {
207+
self::finishRequest($container, $io);
208+
}
209+
}
210+
211+
/**
212+
* Hand the finished response to the client, so that the remaining work of
213+
* this process does not keep it waiting.
214+
*/
215+
private static function finishRequest(DIContainer $container, IOutput $io): void {
216+
// The remaining work must not be cut off when the client goes away
217+
ignore_user_abort(true);
218+
219+
// Otherwise the next request of the same client blocks on the session
220+
// lock for as long as this process keeps working
221+
try {
222+
$container->get(ISession::class)->close();
223+
} catch (ContainerExceptionInterface) {
224+
}
225+
226+
// The client may send a follow-up request that then reads data from
227+
// before the transaction
228+
try {
229+
if ($container->get(IDBConnection::class)->inTransaction()) {
230+
$container->get(LoggerInterface::class)->warning(
231+
'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.',
232+
['app' => 'core'],
233+
);
206234
}
207-
flush();
235+
} catch (ContainerExceptionInterface) {
208236
}
237+
238+
$io->finishRequest();
209239
}
210240
}

‎lib/private/AppFramework/Http/Output.php‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,4 +88,29 @@ public function setCookie($name, $value, $expire, $path, $domain, $secure, $http
8888
'samesite' => $sameSite
8989
]);
9090
}
91+
92+
#[\Override]
93+
public function finishRequest(): bool {
94+
// php-fpm, and the SAPIs aliasing it (recent LiteSpeed, FrankenPHP)
95+
if (function_exists('fastcgi_finish_request')) {
96+
fastcgi_finish_request();
97+
return true;
98+
}
99+
100+
if (function_exists('litespeed_finish_request')) {
101+
litespeed_finish_request();
102+
return true;
103+
}
104+
105+
// mod_php, cgi, cli, … cannot give the connection back to the web server,
106+
// so only push out what we have. ob_flush() instead of ob_end_flush() keeps
107+
// the buffer around for error handling, and is silenced because output
108+
// handlers are allowed to be non-flushable.
109+
if (ob_get_level() > 0) {
110+
@ob_flush();
111+
}
112+
flush();
113+
114+
return false;
115+
}
91116
}

‎lib/public/AppFramework/Http/IOutput.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,4 +57,14 @@ public function setHttpResponseCode($code);
5757
* @since 8.1.0
5858
*/
5959
public function setCookie($name, $value, $expire, $path, $domain, $secure, $httpOnly, $sameSite = 'Lax');
60+
61+
/**
62+
* Send the response produced so far to the client, so the remaining work of
63+
* this process does not delay it. Nothing may be written to the output after.
64+
*
65+
* @return bool true if the connection was closed, false if the response was
66+
* only flushed because the SAPI does not support closing it
67+
* @since 35.0.0
68+
*/
69+
public function finishRequest(): bool;
6070
}

‎lib/public/AppFramework/Http/Response.php‎

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ class Response {
6464

6565
/** @var bool */
6666
private $throttled = false;
67-
private bool $flushEarly = true;
67+
private bool $flushEarly = false;
6868
/** @var array */
6969
private $throttleMetadata = [];
7070

@@ -400,21 +400,33 @@ public function isThrottled() {
400400
}
401401

402402
/**
403-
* Request the response should be flushed to the connected client immediately
403+
* Request that the response is sent to the client as soon as it is complete,
404+
* instead of when the PHP process is done. Enable this only when the
405+
* controller leaves work behind that the client does not have to wait for.
404406
*
405-
* @since 34.0.0
407+
* Things to be aware of before enabling this:
408+
*
409+
* - The session is closed before the response goes out, so anything running
410+
* afterwards can no longer write to it.
411+
* - The status code and the body are final at that point. Errors happening
412+
* afterwards can only be logged, never reported to the client.
413+
* - Nothing may be written to the output afterwards, the Content-Length has
414+
* already been announced.
415+
* - The worker of the web server stays occupied until the process really
416+
* ends, so this improves latency but not throughput.
417+
* - Administrators can turn this off globally with the `flush_response_early`
418+
* system config option
419+
*
420+
* @since 35.0.0
406421
*/
407422
public function setFlushEarly(bool $flushEarly): void {
408423
$this->flushEarly = $flushEarly;
409424
}
410425

411426
/**
412-
* Whether the response should be flushed to the connected client immediately
413-
*
414-
* If not, the response will wait for async actions, e.g. HTTP requests from
415-
* IClientService, to be finished before returning.
427+
* Whether the response should be sent to the client as soon as it is complete
416428
*
417-
* @since 34.0.0
429+
* @since 35.0.0
418430
*/
419431
public function getFlushEarly(): bool {
420432
return $this->flushEarly;

‎psalm.xml‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -129,6 +129,7 @@
129129
<file name="build/stubs/pcntl.php"/>
130130
<file name="build/stubs/zip.php"/>
131131
<file name="build/stubs/psr_container.php"/>
132+
<file name="build/stubs/sapi.php"/>
132133
<file name="3rdparty/sabre/uri/lib/functions.php" />
133134
<file name="build/stubs/app_api.php" />
134135
<file name="build/stubs/php-polyfill.php" />

‎tests/lib/AppFramework/AppTest.php‎

Lines changed: 94 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,11 @@
1414
use OCP\AppFramework\Controller;
1515
use OCP\AppFramework\Http\IOutput;
1616
use OCP\AppFramework\Http\Response;
17+
use OCP\AppFramework\Http\StreamResponse;
18+
use OCP\IConfig;
19+
use OCP\IDBConnection;
20+
use OCP\ISession;
21+
use Psr\Log\LoggerInterface;
1722

1823
function rrmdir($directory) {
1924
$files = array_diff(scandir($directory), ['.','..']);
@@ -86,6 +91,92 @@ public function testControllerNameAndMethodAreBeingPassed(): void {
8691
$this->container);
8792
}
8893

94+
public function testRequestIsNotFinishedEarlyByDefault(): void {
95+
$this->dispatcher->expects($this->once())
96+
->method('dispatch')
97+
->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, new Response()]);
98+
99+
$this->io->expects($this->never())
100+
->method('finishRequest');
101+
102+
App::main($this->controllerName, $this->controllerMethod, $this->container, []);
103+
}
104+
105+
public function testRequestIsFinishedEarlyWhenTheResponseAsksForIt(): void {
106+
$response = new Response();
107+
$response->setFlushEarly(true);
108+
109+
$this->dispatcher->expects($this->once())
110+
->method('dispatch')
111+
->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]);
112+
113+
$session = $this->createMock(ISession::class);
114+
$session->expects($this->once())
115+
->method('close');
116+
$this->container[ISession::class] = $session;
117+
118+
$connection = $this->createMock(IDBConnection::class);
119+
$connection->method('inTransaction')
120+
->willReturn(false);
121+
$this->container[IDBConnection::class] = $connection;
122+
123+
$logger = $this->createMock(LoggerInterface::class);
124+
$logger->expects($this->never())
125+
->method('warning');
126+
$this->container[LoggerInterface::class] = $logger;
127+
128+
$this->io->expects($this->once())
129+
->method('finishRequest');
130+
131+
App::main($this->controllerName, $this->controllerMethod, $this->container, []);
132+
}
133+
134+
public function testRequestIsNotFinishedEarlyWhenDisabledByTheAdmin(): void {
135+
$response = new Response();
136+
$response->setFlushEarly(true);
137+
138+
$this->dispatcher->expects($this->once())
139+
->method('dispatch')
140+
->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]);
141+
142+
$config = $this->createMock(IConfig::class);
143+
$config->method('getSystemValueBool')
144+
->with('flush_response_early', true)
145+
->willReturn(false);
146+
$this->container[IConfig::class] = $config;
147+
148+
$this->io->expects($this->never())
149+
->method('finishRequest');
150+
151+
App::main($this->controllerName, $this->controllerMethod, $this->container, []);
152+
}
153+
154+
public function testOpenTransactionIsLoggedWhenFinishingEarly(): void {
155+
$response = new Response();
156+
$response->setFlushEarly(true);
157+
158+
$this->dispatcher->expects($this->once())
159+
->method('dispatch')
160+
->willReturn(['HTTP/2.0 200 OK', [], [], $this->output, $response]);
161+
162+
$this->container[ISession::class] = $this->createMock(ISession::class);
163+
164+
$connection = $this->createMock(IDBConnection::class);
165+
$connection->method('inTransaction')
166+
->willReturn(true);
167+
$this->container[IDBConnection::class] = $connection;
168+
169+
$logger = $this->createMock(LoggerInterface::class);
170+
$logger->expects($this->once())
171+
->method('warning');
172+
$this->container[LoggerInterface::class] = $logger;
173+
174+
$this->io->expects($this->once())
175+
->method('finishRequest');
176+
177+
App::main($this->controllerName, $this->controllerMethod, $this->container, []);
178+
}
179+
89180
public function testBuildAppNamespace(): void {
90181
$ns = App::buildAppNamespace('someapp');
91182
$this->assertEquals('OCA\Someapp', $ns);
@@ -144,7 +235,9 @@ public function testNoOutput(string $statusCode): void {
144235
}
145236

146237
public function testCallbackIsCalled(): void {
147-
$mock = $this->getMockBuilder('OCP\AppFramework\Http\ICallbackResponse')
238+
// The dispatcher always hands back a Response, not a bare ICallbackResponse
239+
$mock = $this->getMockBuilder(StreamResponse::class)
240+
->disableOriginalConstructor()
148241
->getMock();
149242

150243
$return = ['HTTP/2.0 200 OK', [], [], $this->output, $mock];

‎tests/lib/AppFramework/Http/OutputTest.php‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,4 +27,53 @@ public function testSetReadfileStream(): void {
2727
$output = new Output('');
2828
$output->setReadfile(fopen(__FILE__, 'r'));
2929
}
30+
31+
/** The buffer has to survive the flush so error handling can still use it */
32+
public function testFinishRequestKeepsTheOutputBuffer(): void {
33+
$this->skipIfConnectionCanBeClosed();
34+
$output = new Output('');
35+
36+
ob_start();
37+
$levelBefore = ob_get_level();
38+
$closed = $output->finishRequest();
39+
$levelAfter = ob_get_level();
40+
ob_end_clean();
41+
42+
$this->assertFalse($closed);
43+
$this->assertSame($levelBefore, $levelAfter);
44+
}
45+
46+
/** Draining a handler that refuses to be flushed in a loop would never end */
47+
public function testFinishRequestWithNonFlushableBuffer(): void {
48+
$this->skipIfConnectionCanBeClosed();
49+
$output = new Output('');
50+
51+
ob_start(
52+
static fn (string $buffer): string => $buffer,
53+
0,
54+
PHP_OUTPUT_HANDLER_CLEANABLE | PHP_OUTPUT_HANDLER_REMOVABLE,
55+
);
56+
$levelBefore = ob_get_level();
57+
$closed = $output->finishRequest();
58+
$levelAfter = ob_get_level();
59+
ob_end_clean();
60+
61+
$this->assertFalse($closed);
62+
$this->assertSame($levelBefore, $levelAfter);
63+
}
64+
65+
public function testFinishRequestWithoutAnyOutputBuffer(): void {
66+
$this->skipIfConnectionCanBeClosed();
67+
$output = new Output('');
68+
69+
$level = ob_get_level();
70+
$this->assertFalse($output->finishRequest());
71+
$this->assertSame($level, ob_get_level());
72+
}
73+
74+
private function skipIfConnectionCanBeClosed(): void {
75+
if (function_exists('fastcgi_finish_request') || function_exists('litespeed_finish_request')) {
76+
$this->markTestSkipped('This SAPI closes the connection, which would tear down the output buffer of the test runner');
77+
}
78+
}
3079
}

‎tests/lib/AppFramework/Http/ResponseTest.php‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,4 +259,16 @@ public function testGetThrottleMetadata(): void {
259259
$this->childResponse->throttle(['foo' => 'bar']);
260260
$this->assertSame(['foo' => 'bar'], $this->childResponse->getThrottleMetadata());
261261
}
262+
263+
public function testFlushEarlyIsOptIn(): void {
264+
$this->assertFalse($this->childResponse->getFlushEarly());
265+
}
266+
267+
public function testSetFlushEarly(): void {
268+
$this->childResponse->setFlushEarly(true);
269+
$this->assertTrue($this->childResponse->getFlushEarly());
270+
271+
$this->childResponse->setFlushEarly(false);
272+
$this->assertFalse($this->childResponse->getFlushEarly());
273+
}
262274
}

0 commit comments

Comments
 (0)