Remove type for size arg on Streamer constructor

[artonge]

Resolves#12422 and #15117

On 32 bit systems integers can not be bigger than 2 147 483 647. This is problematic as it throws an error when downloading a folder bigger than 2Go because php will cast the size to float.

[scriptator]

Can somebody review and merge this? For me it seems fine and fixes the problem which I experienced

[nickvergessen]

Urgh, while I can see this solving the problem you have, I guess the issue will pop in quite a lot other locations too. I just think that at some point we just can not manage to stay compatible with 16-bit anymore.

[kesselb]

the issue will pop in quite a lot other locations too.

Yes. Another report for a similar problem is: #13160. Unfortunately https://wiki.php.net/rfc/bigint is still work in progress.

A) Add a library like https://github.com/brick/math and use BigInteger for size attributes.
B) Don't type hint $size

I don't like A nor B 🙈 But there are many raspi instances out there.

[nickvergessen]

Well, you forgot:
C) Don't support mutli GB files on those rapsi instances 🙈

[scriptator]

Urgh, while I can see this solving the problem you have, I guess the issue will pop in quite a lot other locations too.

Totally agree! It would be certainly better to fix the problem at the root

I just think that at some point we just can not manage to stay compatible with 16-bit anymore.

What does this have to do with 16 bit? IMO raspi is running 32 bit which also makes sense when you consider the 2gb limit

[kesselb]

Totally agree! It would be certainly better to fix the problem at the root

Where is the root? PHP not having BigInt? Raspian not using 64 bit? We don't have much options. Change the type of $size for every occurence to this BigInteger library will take months. Not adding type hints for $size is the only way I can think of.

What does this have to do with 16 bit? IMO raspi is running 32 bit which also makes sense when you consider the 2gb limit

Typo ...

[scriptator]

There is of course also another option: officially drop support for 32 bit systems. As I just found out there are 64 bit OS available for the raspberry and even the Raspberrypi foundation is now working on a 64 bit raspbian kernel. If it is clearly stated everywhere that 32 bit does not work properly and raspbian users should install a 64 bit distro I think it should also be okay

[artonge]

I think dropping support for 32-bit systems might be a bit precipitated. 64-bit CPU on a Raspberrypi only came with the version 4, so there will still be a lot of 32-bit rpi for a some years. And the rpi is not the only 32-bit single board computer that is used as a small home server.

So dropping 32-bit support would mean dropping support for a lot of self-hosters.

[kesselb]

cc @rullzer 🏓

[nachoparker]

Raspbian images are still 32 bits userland, even those with a 64 bits kernel.

[kesselb]

Sorry! I forgot the milestone 🙈

[kesselb]

We should probably add a short note to the docblock why typehinting $size is bad.

[go2sh]

I dont think you can change the type to float at all. I had a similar discussions with Morris and the problem is, that you cannot represent every integer with a float exactly especially for big numbers. You might get a value that is some where around the actual size creating a completely new bunch of problems. Out of bound reads, corrupted files, changing files sizes, plus the behavior might be total random as it depends on the actual file size. Because of that, a size in nextcloud should never be float with the consequences for 32 bit instances.

[kesselb]

size creating a completely new bunch of problems. Out of bound reads, corrupted files, changing files sizes, plus the behavior might be total random as it depends on the actual file size.

Probably. But this pr is not about making $size a float. It's about removing the type hint. Requested the storage experts for review ;)

[rullzer]

Yep, we need to think of a better way to tackle this.
As removing it will also blow up in several cases as the resulting zip file will be invalid.

[artonge]

Hi,

Maybe the real solution would be to display an error message to the user explaining why the file or zipped folder can't be downloaded.

Could an exception be thrown here, and caught in an error handler to display the correct error message ?

[skjnldsv]

Ping! :)

[skjnldsv]

Yep, we need to think of a better way to tackle this.
As removing it will also blow up in several cases as the resulting zip file will be invalid.

What shall we do then?

[Wolfgang1966]

How about using tar archives für everything above 4GB? It is already there depending on the client os and it will at least allow to download big archives at all. Ok, Windows does not support tar archives directly, but there are enough tools out there which do. And it is better than being not able to download the file at all.

Tried it with a 9GB archive, was immediately asked to search for a application in the MS app store which offers several of them.

Consider this a workaround, but at least it is (in my opinion) a fair improvement compared to the current situation.

[Wolfgang1966]

How about using tar archives für everything above 4GB? It is already there depending on the client os and it will at least allow to download big archives at all. Ok, Windows does not support tar archives directly, but there are enough tools out there which do. And it is better than being not able to download the file at all.

Tried it with a 9GB archive, was immediately asked to search for a application in the MS app store which offers several of them.

Consider this a workaround, but at least it is (in my opinion) a fair improvement compared to the current situation.

My version of the constructor in Streamer.php now is:

 if ($size > 0 && $size < 4 * 1000 * 1000 * 1000 && $numberOfFiles < 65536) {
$this->streamerInstance = new ZipStreamer(['zip64' => false]);
} else {
$this->streamerInstance = new TarStreamer();
}

[go2sh]

@Wolfgang1966 The problem here is, that the size as float doesn't represent the actual size of the files, as floating point numbers (32bit) are only an aproximation of integers (32bit).

[Wolfgang1966]

@Wolfgang1966 The problem here is, that the size as float doesn't represent the actual size of the files, as floating point numbers (32bit) are only an aproximation of integers (32bit).

This is true, but in that case not relevant. As you can see, the parameter is only used to distuinguish between the usage of 32bit ZIP or anything else. 32bit zip can be used until 4GiB, which is roughly 4.29 GB. As we compare the value to 4.000.000.000 4GB, there is more than enough safety margin for any kind of missing accurancy that might be introduced by using float here.

[kesselb]

public function __construct(IRequest $request, int $size, int $numberOfFiles) {

This code fails if you are on a 32bit operating system and size is bigger than 2147483648 because a bigger value is casted to float and then does not match the type hint anymore and 💥

Whatever you are talking about is a different story and unrelated to this pull request.

[MichaIng]

Btw, Raspberry Pi 2 v1.2 is already ARMv8 capable: https://en.wikipedia.org/wiki/Raspberry_Pi#Specifications
So relevant 32-bit only SBCs are especially the RPi Zero models. But the Raspberry Pi OS 64-bit version is still in beta, so dropping 32-bit support would still affect most RPi users and of course it would also affect a lot of other SBCs, though with a significantly lower user number.

Not representative, but the top 10 systems running DietPi and participating our survey program:

RPi 4 Model B (aarch64)	3607	20%
RPi 4 Model B (armv7l)	1774	10%
RPi 3 Model B (armv7l)	1252	7%
Native PC (x86_64)	1183	6%
RPi Zero W (armv6l)	1179	6%
RPi 3 Model B (aarch64)	1113	6%
Virtual Machine (x86_64)	894	5%
RPi 3 Model B+ (armv7l)	846	4%
RPi 3 Model B+ (aarch64)	778	4%
RPi 2 Model B (armv7l)	711	4%
RPi B (armv6l)	659	3%

armv7l means 32-bit OS version, which is still the majority due to limitations of the 64-bit RPi userland and GPU accelerated software builds.

[MichaIng]

Linking the very same issue with the trash bin: #13160
Possible solution is the same.

[szaimen]

Closing due to nextcloud/documentation#9071