phar: problem with renaming implementation
| From: | Gregory Beaver | Date: | Fri, 21 Mar 2008 19:53:42 +0000 |
| Subject: | phar: problem with renaming implementation | ||
| Groups: | php.pecl.dev | ||
| Request: | Send a blank email to pecl-dev+get-5230@lists.php.net to get a copy of this message | ||
Hi folks,
First, the bad news.
The current implementation of renaming is fatally flawed. In searching
for the source of the memleak, I found that what happens on renaming is
this:
1) phar is added to manifest with the new name
2) phar is not removed from manifest with the old name
on shutdown, we get PHP trying to free both the new and the old name,
hence the double free and memleak. The only way around this is to
completely copy the phar manifest, but then we have a problem with
flushing and it gets really messy really fast.
The good news is that I think there is a simpler way to do this. What
we can do is the following:
1) flush any changes
2) use good old copy() to copy from the old to the new phar (i.e. from
blah.phar to blah.phar.gz)
3) parse the new phar, creating a brand new unconnected manifest entry
with the new name
4) set the compression for the archive (if any)
5) flush the new archive, replacing the copy()ed file (blah.phar.gz in
my example).
This way, the old archive is never touched, and the new one then exists.
Here's some more bad news. Every method that renames will have to
return a new phar object - there is no way to transparently change the
path used by DirectoryIterator once it has been instantiated.
As such, we need to reconsider the API. I'd like to suggest we drop
most of the compression and conversion methods altogether from their
current implementation. Instead, I'd like to create a factory method of
sorts that accepts as arguments an existing Phar object, and some flags
to tell it what to do (compress? convert to zip? etc.)
sample code:
<?php
$phar = new Phar('blah.phar');
$newphar = $phar->convert(Phar::ZIP);
$another = $phar->convert(Phar::TGZ);
$third = $phar->convert(Phar::TAR);
$fourth = $phar->convert(Phar::GZ);
?>
The constants we would use would be Phar::ZIP, Phar::PHAR, Phar::TAR,
Phar::BZ2, Phar::GZ, Phar::DATA and Phar::UNCOMPRESSED. We would allow
Phar::TAR | PHAR::GZ or similar combos, but not Phar::ZIP | PHAR::GZ.
Phar::DATA would be used to convert an active phar into a data-only phar
(i.e. remove the alias and the stub from a tar or zip) and could be
or'ed with Phar::TAR and its GZ/BZ2 constants, or with Phar::ZIP.
compressAllFiles*() would go back to its original purpose of compressing
individual files within the archive (and would throw an exception if
called for tar-based phar archives).
Finally, we need a method for grabbing the phar filename.
Discussion should ensue, and I'm also curious whether you want to take
this on Steph? It should result in MUCH simpler code and logic for our
debugging purposes as well as in the end-user PHP level.
Greg