Skip to content
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.

Sign up for GitHub

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] Abort Streamer-processing when user-connection aborted #42352

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+11 −0
Open

[Fix] Abort Streamer-processing when user-connection aborted #42352

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4a00f33
Abort processing when user-connection aborted
Dec 18, 2023
11b88fc
Fix mis-cased import of Icewind\Streams\CallbackWrapper
Dec 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions lib/private/Streamer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@
*/
namespace OC;

use Icewind\Streams\CallbackWrapper;
use OC\Files\Filesystem;
use OCP\Files\File;
use OCP\Files\Folder;
Expand Down Expand Up @@ -126,6 +127,7 @@ public function addDirRecursive(string $dir, string $internalDir = ''): void {
/** @var LoggerInterface $logger */
$logger = \OC::$server->query(LoggerInterface::class);
foreach ($files as $file) {
if(connection_status() !== CONNECTION_NORMAL) return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(connection_status() !== CONNECTION_NORMAL) return;
if(connection_aborted() === 1) {
break;
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can even skip the === 1:

Suggested change
if(connection_status() !== CONNECTION_NORMAL) return;
if (connection_aborted()) {
break;
}

Same for the following calls to connection_status

if ($file instanceof File) {
try {
$fh = $file->fopen('r');
Expand Down Expand Up @@ -161,6 +163,15 @@ public function addDirRecursive(string $dir, string $internalDir = ''): void {
* @return bool $success
*/
public function addFileFromStream($stream, string $internalName, int|float $size, $time): bool {
if(connection_status() !== CONNECTION_NORMAL) return false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if(connection_status() !== CONNECTION_NORMAL) return false;
if(connection_aborted() === 1) {
return false;
}

// Close file-stream when user-connection closed
$stream = CallbackWrapper::wrap($stream,
Fixed Show fixed Hide fixed
function ($count) use ($stream) {
if (connection_status() !== CONNECTION_NORMAL) {
fclose($stream);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unsure if closing the stream inside the read callback is a good idea.

I suggested using exceptions to bubble up the state properly here: #8161 (comment)

Copy link
Author

@a-bran a-bran Dec 19, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mhh, I thought we can handle this gracefully.
The reading system has to check fefo or if fread succeeded anyways and can then finalize.

In this case e.g. ZipStreamer doesnt catch exceptions so they just bubble to the top and then after everything finished the file is closed by PHP garbage collection.
On theother hand, graceful handling would let ZipStreamer finalize the Zip stream which is a waste of processing.

If we want an exception (so we force cleanup, if any, to execute) we should be consistent and do this in any other case aswell. What do you think?

}
});

$options = [];
if ($time) {
$options = [
Expand Down
Loading