-
Notifications
You must be signed in to change notification settings - Fork 62
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: handle client disconnect properly with ignore_user_abort true #207
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -113,6 +113,12 @@ public static function sendResponse(ResponseInterface $response): void | |
if ($copied <= 0) { | ||
break; | ||
} | ||
// Abort on client disconnect. | ||
// With ignore_user_abort(true), the script is not aborted on client disconnect. | ||
// To avoid reading the entire stream and dismissing the data afterward, check between the chunks if the client is still there. | ||
if (1 === ignore_user_abort() && 1 === connection_aborted()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd assume 0 means false and 1 is true? Or did i mix something up? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. https://www.php.net/manual/en/function.ignore-user-abort.php
When these 2 things both happen, we are deciding, for this "manual" stream-copy loop, we do want to stop feeding the stream copy (even though "the system" has enabed |
||
break; | ||
} | ||
$left -= $copied; | ||
} | ||
} else { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
use Sabre\HTTP; | ||
|
||
include '../bootstrap.php'; | ||
|
||
class DummyStream | ||
{ | ||
private int $position; | ||
|
||
public function stream_open(string $path, string $mode, int $options, ?string &$opened_path): bool | ||
{ | ||
$this->position = 0; | ||
|
||
return true; | ||
} | ||
|
||
public function stream_read(int $count): string | ||
{ | ||
$this->position += $count; | ||
|
||
return random_bytes($count); | ||
} | ||
|
||
public function stream_tell(): int | ||
{ | ||
return $this->position; | ||
} | ||
|
||
public function stream_eof(): bool | ||
{ | ||
return $this->position > 25 * 1024 * 1024; | ||
} | ||
|
||
public function stream_close(): void | ||
{ | ||
file_put_contents(sys_get_temp_dir().'/dummy_stream_read_counter', $this->position); | ||
} | ||
} | ||
|
||
/* | ||
* The DummyStream wrapper has two functions: | ||
* - Provide dummy data. | ||
* - Count how many bytes have been read. | ||
*/ | ||
stream_wrapper_register('dummy', DummyStream::class); | ||
|
||
/* | ||
* Overwrite default connection handling. | ||
* The default behaviour is however for your script to be aborted when the remote client disconnects. | ||
* | ||
* Nextcloud/ownCloud set ignore_user_abort(true) on purpose to work around | ||
* some edge cases where the default behavior would end a script too early. | ||
* | ||
* https://github.com/owncloud/core/issues/22370 | ||
* https://github.com/owncloud/core/pull/26775 | ||
*/ | ||
ignore_user_abort(true); | ||
|
||
$body = fopen('dummy://hello', 'r'); | ||
|
||
$response = new HTTP\Response(); | ||
$response->setStatus(200); | ||
$response->addHeader('Content-Length', 25 * 1024 * 1024); | ||
$response->setBody($body); | ||
|
||
HTTP\Sapi::sendResponse($response); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://www.php.net/manual/en/function.ignore-user-abort.php
This PHP function does return an
int
- looking at its name, I would have assumed it returned abool
!So this code does look OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same experience ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just figured, I should cover this better in PHPStan: phpstan/phpstan-src#2489
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also
connection_aborted
impurity will be covered by phpstan/phpstan-src#2555