ZipStream v3 Beta

[maennchen]

Hi All 👋

Since this package is one of the main dependents on maennchen/zipstream-php and we just released a beta of v3, I would like to ask you for some feedback before we deploy the version as stable.

Since v3 will only support PHP 8.1 and up, you might want to wait until PHP 8.0 is no longer supported to use it.

• Discussion for feedback: Major Release 3 maennchen/ZipStream-PHP#237
• Upgrade instructions: https://github.com/maennchen/ZipStream-PHP/tree/3.0.0-beta.2#upgrade-to-version-300

This release mainly focuses on internal code quality, which should make it easier for outside people to contribute to the project. The code (and the underlying standard) was quite cryptic until now which prevented most people from doing so.

Thanks!

[jszobody]

@maennchen This looks really great, I look forward to reworking this package with v3.

One thing that would really be nice, is if your zipstream-php package provided some nicer way to figure out the final zip filesize in advance (assuming compression is not enabled).

At the moment, I'm doing things like this:

laravel-zipstream/src/ZipStreamFile.php

Lines 46 to 60 in 1b3aea9

	/**
	* This works similar to `processStreamWithZeroHeader` except it fakes sending the data
	* just so we can see what headers/descriptors are generated
	*/
	public function calculate()
	{
	$this->bits |= self::BIT_ZERO_HEADER;
	$this->addFileHeader();
	$this->len = BigInt::init($this->file->getFilesize());
	$this->zlen = BigInt::init($this->file->getFilesize());
	$this->addFileFooter();
	}

Which gets called by the main Zipstream class:

laravel-zipstream/src/ZipStream.php

Lines 327 to 364 in 1b3aea9

	public function predictZipSize(): int
	{
	if (!$this->canPredictZipSize()) {
	return 0;
	}
	if ($this->predictedSize > 0) {
	return $this->predictedSize;
	}
	$this->configureZip64();
	$this->predictedSize = $this->calculateZipSize();
	// It's conceivable that we didn't need Zip64 until the zip headers/decriptors were added.
	// If so, turn on Zip64 and calculate again.
	if(!$this->opt->isEnableZip64() && $this->predictedSize > 0xFFFFFFFF) {
	$this->opt->setEnableZip64(true);
	$this->predictedSize = $this->calculateZipSize();
	}
	return $this->predictedSize;
	}
	/**
	* @return int
	*/
	protected function calculateZipSize(): int
	{
	$this->bytesSent = 0;
	$this->calculateOnly = true;
	$this->queue->map->toZipStreamFile($this)->each->calculate();
	$this->finish();
	$this->calculateOnly = false;
	return $this->queue->sum->getFilesize() + $this->getFinalSize();
	}

It's pretty hackish, I have to fake building the actual zip, just to figure out header sizes and such. My goal is to send a Content-Length header to the browser, so the end-user has a filesize and actual download progress.

I would imagine that users of your package might benefit from the functionality, not just Laravel devs. Would you consider building this right into your package? I think it would end up being cleaner than that fake approach I have to use.

[jszobody]

@maennchen Oof. I see you've made your ZipStream class final, and moved a lot of protected variables to private.

That's going to make it very difficult, maybe even impossible for me to update this package to your v3. That's not very friendly for folks wanting to extend your library. =/

[maennchen]

@jszobody This is exactly why I posted to get some feedback :) Nothing can't be changed at the current state.

Would you consider building this right into your package?

The code does not look very complicated so far. Therefore I'm open to that possibility.

Oof. I see you've made your ZipStream class final, and moved a lot of protected variables to private.

I wanted to be careful to not expose too much internal state to the outside which makes it hard to prevent breaking changes.

I'm open to re-open the interface where it is needed. I would however then prefer to expose a protected internal getter instead of allowing full access to the property itself.

What properties would you need access to?

I cleaned up the visibility a bit already (removed final): maennchen/ZipStream-PHP@d04cca2

[jszobody]

@maennchen If your library provided a way to calculate the final zip size (without compression) after adding all files, that would greatly reduce how much I have to extend/override. The majority of the work that I'm doing in this package is handling the size prediction, which means maintaining my own queue of files, so I can loop through and get sizes, and so on.

So I guess that really drives how much access I need. Ideally, I would love for my package to be stripped down to a lightweight Laravel wrapper that just bootstraps some stuff, sets defaults, and provides a very thin Laravel-esque API on top of your functionality.

[jszobody]

@maennchen Aight I spent a bit of time with it this evening.

In theory, this is my rough process:

1. Keep track of my source files in my own array. I can't addFromStream because you start sending the files as soon as they are added, and I don't want that. I need to queue up files first on my own, figure out filesizes.
2. When ready to send, I need to create a dummy zip with a fake output stream, and empty input streams. I don't want to actually open real file handles yet, or stream in real data. Fake sending out each file and finishing the zip, so I can capture all the headers/footers/cdr stuff. Calculate the total size and send the Content-Length header.
3. Now go back and create a real zip, handing in the real source streams and send out the zip for real.

For the size prediction, with your v3-beta, I think it looks something like this on my end (untested):

// Assume we have a $this->queue with a collection of source file records, which are instances of our own File class with some methods like getSize() and getName(). 

function predictZipSize()
{
    // We are going to create an in-memory output stream to capture all the header/footer/cdr data
    $output = fopen('php://memory','rw');

    // This will be our running total of filesizes. We won't be actually reading in source file
    // data right now, but we need to keep track of it.
    $size = 0;

    // Let's create a dummy zip file, with our output stream
    $zip = new ZipStream(
        outputStream: $output,
        outputName: 'test.zip',
        sendHttpHeaders: false,
        defaultCompressionMethod: CompressionMethod::STORE,
        defaultDeflateLevel: 0
    );

    // This will be our dummy, empty source stream for all files right now
    $stream = fopen('php://memory','r');

    // Loop through all queued files, add them by name, but with our dummy source stream
    // This will write out header/footer/cdr data to our output stream
    foreach($this->queue AS $file) {
        $zip->addFileFromStream(
            fileName: $file->getName(),
            stream: $stream,
        );

        // Keep track of the total file sizes we are examining so far
        $size += $file->getSize();
    }

    // This will send the ending records
    $zip->finish();

    // Ok! Our total zip size should be the size of our output stream + file sizes
    $total = fstat($output)['size'] + $size;

    fclose($output);

    return $total;
}

The big issue I see so far is that I have no way of manipulating your internal offset variable. Since I'm not sending real data in this code, your offset variable will be artificially low. In general that's fine, I'm keeping track of file sizes on my own, and adding it at the end. BUT if we exceed 4gb, you need to add some more Zip64 blocks and that only happens if the offset variable is set.

I looked at manually creating instances of your File class so that I could specify offset, but then there's no way to hand those to your ZipStream class. I'm a bit stumped how to handle this.

Backing up a bit, it would be really helpful if you could separate out the creation of all headers/footers/cdr data from the sending of that data.

I think I would ultimately like to create instances of your File class myself (so I can control the offset), and call getter methods like getFileHeader and getFileFooter and getCdrFile. That would make it easier to do the math on these headers, without having to fake sending them out. Does that make sense?

This is all assuming you don't want to fully build in filesize prediction to your library. I realize that this prediction stuff is highly opinionated, and only works with no compression and zero header set. Not sure if it's something you really want to handle or not.

[maennchen]

@jszobody

If you would like to get access to all headers and footers, They all exist as a separate class. Example:

• https://github.com/maennchen/ZipStream-PHP/blob/main/src/CentralDirectoryFileHeader.php

They are currently marked @internal, but that can be changed.

Exposing an interface to generate them is also not something I'm opposed to. (For example, a simple static function per entry that is also used internally. All parameters could be provided as arguments instead of internal properties.)

Alternate Idea 1:

Since you have to track the offset to detect if Zip64 is required and a few other decisions, it might make sense to actually simulate streaming the archive without actually sending anything, rewind all the input streams and read the total size of written bytes.

That could be achieved relatively easily by implementing the StreamInterface and only tracking the number of written bytes.

• Drawbacks
  • if need to read the whole file stream to determine the size
  • all streams need to support rewinding
• Advantages
  • zero headers & deflation would be supported

Alternate Idea 2:

We could introduce a simulation mode that only calculates the size.

• if not deflated & fstat supported on stream => the size of uncompressed & compressed length is directly known, no need to traverse the file
• else => traverse the whole stream to calculate the sizes and rewind to the start of the stream

No matter which way we decide to go, I'm open to incorporating the size calculation into the project if you help me to do so. It would be appreciated a lot at that point if you would agree to help maintain it as well, is not a requirement though.

[jszobody]

No matter which way we decide to go, I'm open to incorporating the size calculation into the project if you help me to do so. It would be appreciated a lot at that point if you would agree to help maintain it as well, is not a requirement though.

Definitely! I would love to help build this into your library, and maintain it. We are very invested in this package and use it heavily. (Our servers have streamed well over 2,000 zips just in the past 24 hours with this package!)

Finding a way to build the functionality into your library where I don't have to implement weird workarounds would be awesome. Perhaps we should start a discussion over on your repository, and start laying out the way this needs to work, and what options/flags/methods we want to provide for users to make use of it.

...it might make sense to actually simulate streaming the archive without actually sending anything, rewind all the input streams and read the total size of written bytes.

My streams are not always seekable. By default S3 file sources are not seekable. We can make them seekable, but I think that means it caches the data locally in memory or on disk (if I remember correctly, it's been a while since I tried). When streaming a lot of large multi-GB files, this becomes a problem. Oh, and HTTP sources are definitely not seekable. We can't rely on rewinding a stream.

... if not deflated & fstat supported on stream...

My sources are not fstat-able either. To get the filesize for an S3 file we use the dedicated S3 HeadObject API call to get the size. For HTTP sources I use get_headers.

I have to be able to provide filesizes directly to the ZipStream package, without trying to examine the stream itself. I also need to support streams that can't be rewound.

[maennchen]

@jszobody I think you're wrong about the stream not being fstat-able:

https://github.com/aws/aws-sdk-php/blob/master/src/S3/StreamWrapper.php#L243

The condition should be something like this:

compressionMethod === DEFLATE || !fstat($stream)['size'] => needs simulation & rewind.

Given that S3 provides fstat and deflation can be disabled, that should solve your issue I believe.

[jszobody]

@maennchen You're completely right, I missed that. Just tested it, that's awesome.

Next concern though, is that is still requires opening up a stream in order to get the size. And that takes time. We regularly (daily!) zip up thousands of files from S3. As a test, I just ran a loop over an array of 1,000 s3 keys, to see how long it would take to get all the filesizes with fstat. Something along these lines:

$s3Client->registerStreamWrapper();

$totalSize = 0;

foreach($keys AS $key) {
    $handle = fopen("s3://my-bucket/$key", "r");
    $totalSize += fstat($handle)['size'];
    fclose($handle);
}

var_dump($totalSize);

To get the filesizes for 1,000 S3 objects, it took a whopping 2 minutes 34 seconds. 🤯 And that's running on an EC2 instance in the same region as S3.

Now, the exact same issue exists if we try to get file sizes with the S3 API call as well. We used to constantly have issues where a zip request would time out just because it was taking so long just to fetch the file sizes.

The goal, of course, is to start streaming out a zip to the user almost instantly, while also providing a Content-Length header, which means finding filesizes. So we have a problem.

We solved this by storing filesizes in the database. My files table stores the metadata of each file when it is first uploaded and stored in S3. Now I specify each file's size to ZipStream right up front, with no S3 API call or need to open a file stream. I am currently able to calculate the size of a zip almost instantly with this approach. Zips with thousands of files still start streaming very quickly, with the Content-Length header in place.

All that to say: I'm still convinced that for filesize calculation to work, and to scale to thousands of source files, you will need to accept an optional filesize parameter up front. It's reasonable to fall back to fstat if no filesize is provided, but it needs to be available as an option.