[Fix] Abort Streamer-processing when user-connection aborted #42352
Conversation
Signed-off-by: Alexander Brandscheidt <[email protected]>
| @@ -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; | |||
There was a problem hiding this comment.
| if(connection_status() !== CONNECTION_NORMAL) return; | |
| if(connection_aborted() === 1) { | |
| break; | |
| } |
There was a problem hiding this comment.
I think we can even skip the === 1:
| if(connection_status() !== CONNECTION_NORMAL) return; | |
| if (connection_aborted()) { | |
| break; | |
| } |
Same for the following calls to connection_status
| $stream = CallbackWrapper::wrap($stream, | ||
| function ($count) use ($stream) { | ||
| if (connection_status() !== CONNECTION_NORMAL) { | ||
| fclose($stream); |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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?
| @@ -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; | |||
There was a problem hiding this comment.
| if(connection_status() !== CONNECTION_NORMAL) return false; | |
| if(connection_aborted() === 1) { | |
| return false; | |
| } |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Alexander Brandscheidt <[email protected]>
|
@kesselb Hey, some time has passed, sorry if i am bothing you by pinging. |
skjnldsv
added
2. developing
Summary
Streamer, which builds compressed archives to stream to the browser (e.g. for downloading folders), didnt have checks when the browser aborts the connection (e.g. download cancelled).
This PR adds checks at certain positions:
addDirRecursiveto avoid opening files or further traversing directories, even though writing is impossibleaddFileFromStreamconnection_status()only updates when data is attempted to be send to the browser (e.g. withflush())Todo