Parallelize zip extraction using async unzip processes

src/Composer/Downloader/ArchiveDownloader.php

@@ -16,6 +16,7 @@ use Composer\Package\PackageInterface;
 use Symfony\Component\Finder\Finder;
 use Composer\IO\IOInterface;
 use Composer\Exception\IrrecoverableDownloadException;
+use React\Promise\PromiseInterface;
 
 /**
  * Base downloader for archives
@@ -60,26 +61,62 @@ abstract class ArchiveDownloader extends FileDownloader
             $temporaryDir = $this->config->get('vendor-dir').'/composer/'.substr(md5(uniqid('', true)), 0, 8);
         } while (is_dir($temporaryDir));
 
+        $this->addCleanupPath($package, $temporaryDir);
+
+        $this->filesystem->ensureDirectoryExists($temporaryDir);
         $fileName = $this->getFileName($package, $path);
 
-        try {
+        $filesystem = $this->filesystem;
-            $this->filesystem->ensureDirectoryExists($temporaryDir);
+        $self = $this;
-            try {
-                $this->extract($package, $fileName, $temporaryDir);
-            } catch (\Exception $e) {
-                // remove cache if the file was corrupted
-                parent::clearLastCacheWrite($package);
-                throw $e;
-            }
 
-            $this->filesystem->unlink($fileName);
+        $cleanup = function () use ($path, $filesystem, $temporaryDir, $package, $self) {
+            // remove cache if the file was corrupted
+            $self->clearLastCacheWrite($package);
+
+            // clean up
+            $filesystem->removeDirectory($path);
+            $filesystem->removeDirectory($temporaryDir);
+            $self->removeCleanupPath($package, $temporaryDir);
+        };
+
+        $promise = null;
+        try {
+            $promise = $this->extract($package, $fileName, $temporaryDir);
+        } catch (\Exception $e) {
+            $cleanup();
+            throw $e;
+        }
+
+        if (!$promise instanceof PromiseInterface) {
+            $promise = \React\Promise\resolve();
+        }
+
+        return $promise->then(function () use ($self, $package, $filesystem, $fileName, $temporaryDir, $path) {
+            $filesystem->unlink($fileName);
+
+            /**
+             * Returns the folder content, excluding dotfiles
+             *
+             * @param  string         $dir Directory
+             * @return \SplFileInfo[]
+             */
+            $getFolderContent = function ($dir) {
+                $finder = Finder::create()
+                    ->ignoreVCS(false)
+                    ->ignoreDotFiles(false)
+                    ->notName('.DS_Store')
+                    ->depth(0)
+                    ->in($dir);
+
+                return iterator_to_array($finder);
+            };
 
             $renameAsOne = false;
-            if (!file_exists($path) || ($this->filesystem->isDirEmpty($path) && $this->filesystem->removeDirectory($path))) {
+            if (!file_exists($path) || ($filesystem->isDirEmpty($path) && $filesystem->removeDirectory($path))) {
                 $renameAsOne = true;
             }
 
-            $contentDir = $this->getFolderContent($temporaryDir);
+            $contentDir = $getFolderContent($temporaryDir);
             $singleDirAtTopLevel = 1 === count($contentDir) && is_dir(reset($contentDir));
 
             if ($renameAsOne) {
@@ -89,28 +126,27 @@ abstract class ArchiveDownloader extends FileDownloader
                 } else {
                     $extractedDir = $temporaryDir;
                 }
-                $this->filesystem->rename($extractedDir, $path);
+                $filesystem->rename($extractedDir, $path);
             } else {
                 // only one dir in the archive, extract its contents out of it
                 if ($singleDirAtTopLevel) {
-                    $contentDir = $this->getFolderContent((string) reset($contentDir));
+                    $contentDir = $getFolderContent((string) reset($contentDir));
                 }
 
                 // move files back out of the temp dir
                 foreach ($contentDir as $file) {
                     $file = (string) $file;
-                    $this->filesystem->rename($file, $path . '/' . basename($file));
+                    $filesystem->rename($file, $path . '/' . basename($file));
                 }
             }
 
-            $this->filesystem->removeDirectory($temporaryDir);
+            $filesystem->removeDirectory($temporaryDir);
-        } catch (\Exception $e) {
+            $self->removeCleanupPath($package, $temporaryDir);
-            // clean up
+        }, function ($e) use ($cleanup) {
-            $this->filesystem->removeDirectory($path);
+            $cleanup();
-            $this->filesystem->removeDirectory($temporaryDir);
 
             throw $e;
-        }
+        });
     }
 
     /**
@@ -119,25 +155,8 @@ abstract class ArchiveDownloader extends FileDownloader
      * @param string $file Extracted file
      * @param string $path Directory
      *
+     * @return PromiseInterface|null
      * @throws \UnexpectedValueException If can not extract downloaded file to path
      */
     abstract protected function extract(PackageInterface $package, $file, $path);
-
-    /**
-     * Returns the folder content, excluding dotfiles
-     *
-     * @param  string         $dir Directory
-     * @return \SplFileInfo[]
-     */
-    private function getFolderContent($dir)
-    {
-        $finder = Finder::create()
-            ->ignoreVCS(false)
-            ->ignoreDotFiles(false)
-            ->notName('.DS_Store')
-            ->depth(0)
-            ->in($dir);
-
-        return iterator_to_array($finder);
-    }
 }

src/Composer/Downloader/ZipDownloader.php

@@ -86,9 +86,8 @@ class ZipDownloader extends ArchiveDownloader
      * @param  string $file         File to extract
      * @param  string $path         Path where to extract file
      * @param  bool   $isLastChance If true it is called as a fallback and should throw an exception
-     * @return bool   Success status
      */
-    protected function extractWithSystemUnzip($file, $path, $isLastChance)
+    private function extractWithSystemUnzip(PackageInterface $package, $file, $path, $isLastChance, $async = false)
     {
         if (!self::$hasZipArchive) {
             // Force Exception throwing if the Other alternative is not available
@@ -98,18 +97,47 @@ class ZipDownloader extends ArchiveDownloader
         if (!self::$hasSystemUnzip && !$isLastChance) {
             // This was call as the favorite extract way, but is not available
             // We switch to the alternative
-            return $this->extractWithZipArchive($file, $path, true);
+            return $this->extractWithZipArchive($package, $file, $path, true);
+        }
+
+        // When called after a ZipArchive failed, perhaps there is some files to overwrite
+        $overwrite = $isLastChance ? '-o' : '';
+        $command = 'unzip -qq '.$overwrite.' '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path);
+
+        if ($async) {
+            $self = $this;
+            $io = $this->io;
+            $tryFallback = function ($processError) use ($isLastChance, $io, $self, $file, $path, $package) {
+                if ($isLastChance) {
+                    throw $processError;
+                }
+
+                $io->writeError('    <warning>'.$processError->getMessage().'</warning>');
+                $io->writeError('    The archive may contain identical file names with different capitalization (which fails on case insensitive filesystems)');
+                $io->writeError('    Unzip with unzip command failed, falling back to ZipArchive class');
+
+                return $self->extractWithZipArchive($package, $file, $path, true);
+            };
+
+            try {
+                $promise = $this->process->executeAsync($command);
+
+                return $promise->then(function ($process) use ($tryFallback, $command, $package) {
+                    if (!$process->isSuccessful()) {
+                        return $tryFallback(new \RuntimeException('Failed to extract '.$package->getName().': ('.$process->getExitCode().') '.$command."\n\n".$process->getErrorOutput()));
+                    }
+                });
+            } catch (\Exception $e) {
+                return $tryFallback($e);
+            } catch (\Throwable $e) {
+                return $tryFallback($e);
+            }
         }
 
         $processError = null;
-        // When called after a ZipArchive failed, perhaps there is some files to overwrite
-        $overwrite = $isLastChance ? '-o' : '';
-
-        $command = 'unzip -qq '.$overwrite.' '.ProcessExecutor::escape($file).' -d '.ProcessExecutor::escape($path);
-
         try {
             if (0 === $exitCode = $this->process->execute($command, $ignoredOutput)) {
-                return true;
+                return \React\Promise\resolve();
             }
 
             $processError = new \RuntimeException('Failed to execute ('.$exitCode.') '.$command."\n\n".$this->process->getErrorOutput());
@@ -121,11 +149,11 @@ class ZipDownloader extends ArchiveDownloader
             throw $processError;
         }
 
-        $this->io->writeError('    '.$processError->getMessage());
+        $this->io->writeError('    <warning>'.$processError->getMessage().'</warning>');
         $this->io->writeError('    The archive may contain identical file names with different capitalization (which fails on case insensitive filesystems)');
         $this->io->writeError('    Unzip with unzip command failed, falling back to ZipArchive class');
 
-        return $this->extractWithZipArchive($file, $path, true);
+        return $this->extractWithZipArchive($package, $file, $path, true);
     }
 
     /**
@@ -134,9 +162,11 @@ class ZipDownloader extends ArchiveDownloader
      * @param  string $file         File to extract
      * @param  string $path         Path where to extract file
      * @param  bool   $isLastChance If true it is called as a fallback and should throw an exception
-     * @return bool   Success status
+     *
+     * TODO v3 should make this private once we can drop PHP 5.3 support
+     * @protected
      */
-    protected function extractWithZipArchive($file, $path, $isLastChance)
+    public function extractWithZipArchive(PackageInterface $package, $file, $path, $isLastChance)
     {
         if (!self::$hasSystemUnzip) {
             // Force Exception throwing if the Other alternative is not available
@@ -146,7 +176,7 @@ class ZipDownloader extends ArchiveDownloader
         if (!self::$hasZipArchive && !$isLastChance) {
             // This was call as the favorite extract way, but is not available
             // We switch to the alternative
-            return $this->extractWithSystemUnzip($file, $path, true);
+            return $this->extractWithSystemUnzip($package, $file, $path, true);
         }
 
         $processError = null;
@@ -159,7 +189,7 @@ class ZipDownloader extends ArchiveDownloader
                 if (true === $extractResult) {
                     $zipArchive->close();
 
-                    return true;
+                    return \React\Promise\resolve();
                 }
 
                 $processError = new \RuntimeException(rtrim("There was an error extracting the ZIP file, it is either corrupted or using an invalid format.\n"));
@@ -170,16 +200,18 @@ class ZipDownloader extends ArchiveDownloader
             $processError = new \RuntimeException('The archive may contain identical file names with different capitalization (which fails on case insensitive filesystems): '.$e->getMessage(), 0, $e);
         } catch (\Exception $e) {
             $processError = $e;
+        } catch (\Throwable $e) {
+            $processError = $e;
         }
 
         if ($isLastChance) {
             throw $processError;
         }
 
-        $this->io->writeError('    '.$processError->getMessage());
+        $this->io->writeError('    <warning>'.$processError->getMessage().'</warning>');
         $this->io->writeError('    Unzip with ZipArchive class failed, falling back to unzip command');
 
-        return $this->extractWithSystemUnzip($file, $path, true);
+        return $this->extractWithSystemUnzip($package, $file, $path, true);
     }
 
     /**
@@ -192,10 +224,10 @@ class ZipDownloader extends ArchiveDownloader
     {
         // Each extract calls its alternative if not available or fails
         if (self::$isWindows) {
-            $this->extractWithZipArchive($file, $path, false);
+            return $this->extractWithZipArchive($package, $file, $path, false);
-        } else {
-            $this->extractWithSystemUnzip($file, $path, false);
         }
+
+        return $this->extractWithSystemUnzip($package, $file, $path, false, true);
     }
 
     /**

tests/Composer/Test/Downloader/ZipDownloaderTest.php

@@ -179,37 +179,65 @@ class ZipDownloaderTest extends TestCase
 
     /**
      * @expectedException \Exception
-     * @expectedExceptionMessage Failed to execute (1) unzip
+     * @expectedExceptionMessage Failed to extract : (1) unzip
      */
     public function testSystemUnzipOnlyFailed()
     {
-        if (!class_exists('ZipArchive')) {
+        $this->setPrivateProperty('isWindows', false);
-            $this->markTestSkipped('zip extension missing');
-        }
-
         $this->setPrivateProperty('hasSystemUnzip', true);
         $this->setPrivateProperty('hasZipArchive', false);
+
+        $procMock = $this->getMockBuilder('Symfony\Component\Process\Process')->disableOriginalConstructor()->getMock();
+        $procMock->expects($this->any())
+            ->method('getExitCode')
+            ->will($this->returnValue(1));
+        $procMock->expects($this->any())
+            ->method('isSuccessful')
+            ->will($this->returnValue(false));
+        $procMock->expects($this->any())
+            ->method('getErrorOutput')
+            ->will($this->returnValue('output'));
+
         $processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock();
         $processExecutor->expects($this->at(0))
-            ->method('execute')
+            ->method('executeAsync')
-            ->will($this->returnValue(1));
+            ->will($this->returnValue(\React\Promise\resolve($procMock)));
 
         $downloader = new MockedZipDownloader($this->io, $this->config, $this->httpDownloader, null, null, null, $processExecutor);
-        $downloader->extract($this->package, 'testfile.zip', 'vendor/dir');
+        $promise = $downloader->extract($this->package, 'testfile.zip', 'vendor/dir');
+        $e = null;
+        $promise->then(function () {
+            // noop
+        }, function ($ex) use (&$e) {
+            $e = $ex;
+        });
+
+        if ($e) {
+            throw $e;
+        }
     }
 
     public function testSystemUnzipOnlyGood()
     {
-        if (!class_exists('ZipArchive')) {
+        $this->setPrivateProperty('isWindows', false);
-            $this->markTestSkipped('zip extension missing');
-        }
-
         $this->setPrivateProperty('hasSystemUnzip', true);
         $this->setPrivateProperty('hasZipArchive', false);
+
+        $procMock = $this->getMockBuilder('Symfony\Component\Process\Process')->disableOriginalConstructor()->getMock();
+        $procMock->expects($this->any())
+            ->method('getExitCode')
+            ->will($this->returnValue(0));
+        $procMock->expects($this->any())
+            ->method('isSuccessful')
+            ->will($this->returnValue(true));
+        $procMock->expects($this->any())
+            ->method('getErrorOutput')
+            ->will($this->returnValue('output'));
+
         $processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock();
         $processExecutor->expects($this->at(0))
-            ->method('execute')
+            ->method('executeAsync')
-            ->will($this->returnValue(0));
+            ->will($this->returnValue(\React\Promise\resolve($procMock)));
 
         $downloader = new MockedZipDownloader($this->io, $this->config, $this->httpDownloader, null, null, null, $processExecutor);
         $downloader->extract($this->package, 'testfile.zip', 'vendor/dir');
@@ -225,10 +253,21 @@ class ZipDownloaderTest extends TestCase
         $this->setPrivateProperty('hasSystemUnzip', true);
         $this->setPrivateProperty('hasZipArchive', true);
 
+        $procMock = $this->getMockBuilder('Symfony\Component\Process\Process')->disableOriginalConstructor()->getMock();
+        $procMock->expects($this->any())
+            ->method('getExitCode')
+            ->will($this->returnValue(1));
+        $procMock->expects($this->any())
+            ->method('isSuccessful')
+            ->will($this->returnValue(false));
+        $procMock->expects($this->any())
+            ->method('getErrorOutput')
+            ->will($this->returnValue('output'));
+
         $processExecutor = $this->getMockBuilder('Composer\Util\ProcessExecutor')->getMock();
         $processExecutor->expects($this->at(0))
-            ->method('execute')
+            ->method('executeAsync')
-            ->will($this->returnValue(1));
+            ->will($this->returnValue(\React\Promise\resolve($procMock)));
 
         $zipArchive = $this->getMockBuilder('ZipArchive')->getMock();
         $zipArchive->expects($this->at(0))
@@ -350,6 +389,6 @@ class MockedZipDownloader extends ZipDownloader
 
     public function extract(PackageInterface $package, $file, $path)
     {
-        parent::extract($package, $file, $path);
+        return parent::extract($package, $file, $path);
     }
 }