From 16592ca6b19b938b4ef05062c71014ba411e755e Mon Sep 17 00:00:00 2001 From: Jan-Marten de Boer Date: Thu, 11 Oct 2018 14:42:08 +0200 Subject: [PATCH 1/3] Add regression test for issue #25 A regression test is added, confirming the issue that a specific package suffers from out of memory errors when being analyzed by Dependency Guard. --- .../ComposerTestEnvironmentTrait.php | 84 ++++ tests/Regression/Issue21/Issue21Test.php | 35 +- tests/Regression/Issue25/Issue25Test.php | 45 +++ tests/Regression/Issue25/composer.json | 11 + tests/Regression/Issue25/composer.lock | 359 ++++++++++++++++++ 5 files changed, 501 insertions(+), 33 deletions(-) create mode 100644 tests/Regression/ComposerTestEnvironmentTrait.php create mode 100644 tests/Regression/Issue25/Issue25Test.php create mode 100644 tests/Regression/Issue25/composer.json create mode 100644 tests/Regression/Issue25/composer.lock diff --git a/tests/Regression/ComposerTestEnvironmentTrait.php b/tests/Regression/ComposerTestEnvironmentTrait.php new file mode 100644 index 0000000..f998eaa --- /dev/null +++ b/tests/Regression/ComposerTestEnvironmentTrait.php @@ -0,0 +1,84 @@ +workingDirectory === null) { + $reflection = new ReflectionObject($this); + + $this->workingDirectory = dirname($reflection->getFileName()); + } + + return $this->workingDirectory; + } + + /** + * Create a composer process for the given command. + * + * @param string $command + * @param string ...$arguments + * + * @return Process + */ + protected function createComposerProcess( + string $command, + string ...$arguments + ): Process { + return new Process( + array_merge( + [ + trim(`which composer` ?? `which composer.phar`), + $command, + ], + $arguments, + [ + '--quiet', + '--working-dir', + $this->getWorkingDirectory() + ] + ) + ); + } + + /** + * @return void + */ + public function setUp(): void + { + $process = $this->createComposerProcess('install'); + + if ($process->run() !== 0) { + self::markTestSkipped( + $process->getErrorOutput() + ); + } + } + + /** + * @return void + */ + public function tearDown(): void + { + $filesystem = new Filesystem(); + $filesystem->removeDirectory( + $this->getWorkingDirectory() . DIRECTORY_SEPARATOR . 'vendor' + ); + } +} diff --git a/tests/Regression/Issue21/Issue21Test.php b/tests/Regression/Issue21/Issue21Test.php index a4664ab..365e206 100644 --- a/tests/Regression/Issue21/Issue21Test.php +++ b/tests/Regression/Issue21/Issue21Test.php @@ -2,7 +2,7 @@ namespace Mediact\DependencyGuard\Tests\Regression\Issue21; -use Composer\Util\Filesystem; +use Mediact\DependencyGuard\Tests\Regression\ComposerTestEnvironmentTrait; use PHPUnit\Framework\TestCase; use Symfony\Component\Process\Process; @@ -11,27 +11,7 @@ */ class Issue21Test extends TestCase { - /** - * @return void - */ - public function setUp(): void - { - $process = new Process( - [ - trim(`which composer` ?? `which composer.phar`), - 'install', - '--quiet', - '--working-dir', - __DIR__ - ] - ); - - if ($process->run() !== 0) { - self::markTestSkipped( - $process->getErrorOutput() - ); - } - } + use ComposerTestEnvironmentTrait; /** * @return void @@ -51,15 +31,4 @@ public function testNoFatalsBecauseOfConflicts(): void self::assertNotContains('Fatal error:', $process->getErrorOutput()); } - - /** - * @return void - */ - public function tearDown(): void - { - $filesystem = new Filesystem(); - $filesystem->removeDirectory( - __DIR__ . DIRECTORY_SEPARATOR . 'vendor' - ); - } } diff --git a/tests/Regression/Issue25/Issue25Test.php b/tests/Regression/Issue25/Issue25Test.php new file mode 100644 index 0000000..1c554eb --- /dev/null +++ b/tests/Regression/Issue25/Issue25Test.php @@ -0,0 +1,45 @@ +run(); + + $this->assertNotContains( + 'Allowed memory size', + $process->getErrorOutput(), + 'Dependency guard ran out of memory.' + ); + } +} diff --git a/tests/Regression/Issue25/composer.json b/tests/Regression/Issue25/composer.json new file mode 100644 index 0000000..7a606f4 --- /dev/null +++ b/tests/Regression/Issue25/composer.json @@ -0,0 +1,11 @@ +{ + "name": "issue25/test", + "require": { + "php-twinfield/twinfield": "dev-master#04c1b248efc9268db2ad173a9305fa9c21970df8" + }, + "autoload": { + "psr-4": { + "PhpTwinfield\\": "vendor/php-twinfield/twinfield/src" + } + } +} diff --git a/tests/Regression/Issue25/composer.lock b/tests/Regression/Issue25/composer.lock new file mode 100644 index 0000000..1612eda --- /dev/null +++ b/tests/Regression/Issue25/composer.lock @@ -0,0 +1,359 @@ +{ + "_readme": [ + "This file locks the dependencies of your project to a known state", + "Read more about it at https://getcomposer.org/doc/01-basic-usage.md#installing-dependencies", + "This file is @generated automatically" + ], + "content-hash": "64f1bcfc7726d13782af6d2f94af0b0f", + "packages": [ + { + "name": "moneyphp/money", + "version": "v3.1.3", + "source": { + "type": "git", + "url": "https://github.com/moneyphp/money.git", + "reference": "5e6a3c98ba2cb190d48d35656967eacf30716034" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/moneyphp/money/zipball/5e6a3c98ba2cb190d48d35656967eacf30716034", + "reference": "5e6a3c98ba2cb190d48d35656967eacf30716034", + "shasum": "" + }, + "require": { + "php": ">=5.6" + }, + "require-dev": { + "cache/taggable-cache": "^0.4.0", + "doctrine/instantiator": "^1.0.5", + "ext-bcmath": "*", + "ext-gmp": "*", + "ext-intl": "*", + "florianv/swap": "^3.0", + "leanphp/phpspec-code-coverage": "^3.0 || ^4.0", + "moneyphp/iso-currencies": "^3.0", + "php-http/message": "^1.4", + "php-http/mock-client": "^0.3.3", + "phpspec/phpspec": "^3.0", + "phpunit/phpunit": "^5", + "psr/cache": "^1.0", + "sllh/php-cs-fixer-styleci-bridge": "^2.1", + "symfony/phpunit-bridge": "^4" + }, + "suggest": { + "ext-bcmath": "Calculate without integer limits", + "ext-gmp": "Calculate without integer limits", + "ext-intl": "Format Money objects with intl", + "florianv/swap": "Exchange rates library for PHP", + "psr/cache-implementation": "Used for Currency caching" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "3.0-dev" + } + }, + "autoload": { + "psr-4": { + "Money\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Mathias Verraes", + "email": "mathias@verraes.net", + "homepage": "http://verraes.net" + }, + { + "name": "Frederik Bosch", + "email": "f.bosch@genkgo.nl" + }, + { + "name": "Márk Sági-Kazár", + "email": "mark.sagi-kazar@gmail.com" + } + ], + "description": "PHP implementation of Fowler's Money pattern", + "homepage": "http://verraes.net/2011/04/fowler-money-pattern-in-php/", + "keywords": [ + "Value Object", + "money", + "vo" + ], + "time": "2018-02-16T11:04:16+00:00" + }, + { + "name": "myclabs/php-enum", + "version": "1.6.2", + "source": { + "type": "git", + "url": "https://github.com/myclabs/php-enum.git", + "reference": "ca2f4090a7ecae6f0c67fc9bd07cfb51cdf04219" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/myclabs/php-enum/zipball/ca2f4090a7ecae6f0c67fc9bd07cfb51cdf04219", + "reference": "ca2f4090a7ecae6f0c67fc9bd07cfb51cdf04219", + "shasum": "" + }, + "require": { + "php": ">=5.4" + }, + "require-dev": { + "phpunit/phpunit": "^4.8.35|^5.7|^6.0", + "squizlabs/php_codesniffer": "1.*" + }, + "type": "library", + "autoload": { + "psr-4": { + "MyCLabs\\Enum\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "PHP Enum contributors", + "homepage": "https://github.com/myclabs/php-enum/graphs/contributors" + } + ], + "description": "PHP Enum implementation", + "homepage": "http://github.com/myclabs/php-enum", + "keywords": [ + "enum" + ], + "time": "2018-08-01T21:05:54+00:00" + }, + { + "name": "php-twinfield/twinfield", + "version": "dev-master", + "source": { + "type": "git", + "url": "https://github.com/php-twinfield/twinfield.git", + "reference": "04c1b248efc9268db2ad173a9305fa9c21970df8" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-twinfield/twinfield/zipball/04c1b248efc9268db2ad173a9305fa9c21970df8", + "reference": "04c1b248efc9268db2ad173a9305fa9c21970df8", + "shasum": "" + }, + "require": { + "ext-dom": "*", + "ext-json": "*", + "ext-soap": "*", + "moneyphp/money": "^3.0", + "myclabs/php-enum": "^1.5", + "php": ">=7.1", + "psr/http-message": "^1.0", + "psr/log": "^1.0", + "webmozart/assert": "^1.2" + }, + "conflict": { + "pronamic/twinfield": "*" + }, + "replace": { + "pronamic/twinfield": "*" + }, + "require-dev": { + "eloquent/liberator": "^2.0", + "league/oauth2-client": "^2.2", + "mediact/dependency-guard": "^1.0", + "phpstan/phpstan": "^0.10.1", + "phpunit/phpunit": "^7.3" + }, + "suggest": { + "league/oauth2-client": "If you want to use OAuth to login" + }, + "type": "library", + "autoload": { + "psr-4": { + "PhpTwinfield\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "GPL-3.0-or-later" + ], + "authors": [ + { + "name": "Remco Tolsma", + "email": "remcotolsma@pronamic.nl" + }, + { + "name": "Leon Rowland", + "email": "leon@pronamic.nl" + } + ], + "description": "Library for using the Twinfield Soap Service.", + "homepage": "https://github.com/php-twinfield/twinfield", + "keywords": [ + "twinfield" + ], + "time": "2018-10-02T15:48:25+00:00" + }, + { + "name": "psr/http-message", + "version": "1.0.1", + "source": { + "type": "git", + "url": "https://github.com/php-fig/http-message.git", + "reference": "f6561bf28d520154e4b0ec72be95418abe6d9363" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/http-message/zipball/f6561bf28d520154e4b0ec72be95418abe6d9363", + "reference": "f6561bf28d520154e4b0ec72be95418abe6d9363", + "shasum": "" + }, + "require": { + "php": ">=5.3.0" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.0.x-dev" + } + }, + "autoload": { + "psr-4": { + "Psr\\Http\\Message\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "PHP-FIG", + "homepage": "http://www.php-fig.org/" + } + ], + "description": "Common interface for HTTP messages", + "homepage": "https://github.com/php-fig/http-message", + "keywords": [ + "http", + "http-message", + "psr", + "psr-7", + "request", + "response" + ], + "time": "2016-08-06T14:39:51+00:00" + }, + { + "name": "psr/log", + "version": "1.0.2", + "source": { + "type": "git", + "url": "https://github.com/php-fig/log.git", + "reference": "4ebe3a8bf773a19edfe0a84b6585ba3d401b724d" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/php-fig/log/zipball/4ebe3a8bf773a19edfe0a84b6585ba3d401b724d", + "reference": "4ebe3a8bf773a19edfe0a84b6585ba3d401b724d", + "shasum": "" + }, + "require": { + "php": ">=5.3.0" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.0.x-dev" + } + }, + "autoload": { + "psr-4": { + "Psr\\Log\\": "Psr/Log/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "PHP-FIG", + "homepage": "http://www.php-fig.org/" + } + ], + "description": "Common interface for logging libraries", + "homepage": "https://github.com/php-fig/log", + "keywords": [ + "log", + "psr", + "psr-3" + ], + "time": "2016-10-10T12:19:37+00:00" + }, + { + "name": "webmozart/assert", + "version": "1.3.0", + "source": { + "type": "git", + "url": "https://github.com/webmozart/assert.git", + "reference": "0df1908962e7a3071564e857d86874dad1ef204a" + }, + "dist": { + "type": "zip", + "url": "https://api.github.com/repos/webmozart/assert/zipball/0df1908962e7a3071564e857d86874dad1ef204a", + "reference": "0df1908962e7a3071564e857d86874dad1ef204a", + "shasum": "" + }, + "require": { + "php": "^5.3.3 || ^7.0" + }, + "require-dev": { + "phpunit/phpunit": "^4.6", + "sebastian/version": "^1.0.1" + }, + "type": "library", + "extra": { + "branch-alias": { + "dev-master": "1.3-dev" + } + }, + "autoload": { + "psr-4": { + "Webmozart\\Assert\\": "src/" + } + }, + "notification-url": "https://packagist.org/downloads/", + "license": [ + "MIT" + ], + "authors": [ + { + "name": "Bernhard Schussek", + "email": "bschussek@gmail.com" + } + ], + "description": "Assertions to validate method input/output with nice error messages.", + "keywords": [ + "assert", + "check", + "validate" + ], + "time": "2018-01-29T19:49:41+00:00" + } + ], + "packages-dev": [], + "aliases": [], + "minimum-stability": "stable", + "stability-flags": { + "php-twinfield/twinfield": 20 + }, + "prefer-stable": false, + "prefer-lowest": false, + "platform": [], + "platform-dev": [] +} From b86fe8bf9feeb97beb200c8c1bc97b6434424845 Mon Sep 17 00:00:00 2001 From: Jan-Marten de Boer Date: Thu, 11 Oct 2018 15:15:54 +0200 Subject: [PATCH 2/3] Fix #25 - Out of memory errors When a ReflectionClass from BetterReflection was requested statically, it created a new instance of BetterReflection under the hood. This instance of BetterReflection created a new PHP parser under the hood, every time. So, every time a PHP symbol was reflected upon, the whole parser was instantiated, which was really memory consuming. In the case of issue #25, this resulted in an out of memory error, requesting over 1GB of memory to run Dependency Guard. By preventing zealous object instantiation, the memory consumption is now below a set threshold of 15MB of memory for the package that was affected in issue #25. --- src/Candidate/CandidateExtractor.php | 6 ++- src/Php/Filter/UserDefinedSymbolFilter.php | 6 ++- src/Php/SymbolExtractor.php | 4 +- src/Reflection/ReflectionTrait.php | 44 ++++++++++++++++++++++ tests/Reflection/ReflectionTraitTest.php | 33 ++++++++++++++++ tests/Regression/Issue25/Issue25Test.php | 2 +- 6 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 src/Reflection/ReflectionTrait.php create mode 100644 tests/Reflection/ReflectionTraitTest.php diff --git a/src/Candidate/CandidateExtractor.php b/src/Candidate/CandidateExtractor.php index 51ced65..e065041 100644 --- a/src/Candidate/CandidateExtractor.php +++ b/src/Candidate/CandidateExtractor.php @@ -11,10 +11,12 @@ use Mediact\DependencyGuard\Php\SymbolInterface; use Mediact\DependencyGuard\Php\SymbolIterator; use Mediact\DependencyGuard\Php\SymbolIteratorInterface; -use Roave\BetterReflection\Reflection\ReflectionClass; +use Mediact\DependencyGuard\Reflection\ReflectionTrait; class CandidateExtractor implements CandidateExtractorInterface { + use ReflectionTrait; + /** * Extract violation candidates from the given Composer instance and symbols. * @@ -100,7 +102,7 @@ private function extractPackage( $name = $symbol->getName(); if (!array_key_exists($name, $packagesPerSymbol)) { - $reflection = ReflectionClass::createFromName($name); + $reflection = $this->getClassReflection($name); $file = $reflection->getFileName(); // This happens for symbols in the current package. diff --git a/src/Php/Filter/UserDefinedSymbolFilter.php b/src/Php/Filter/UserDefinedSymbolFilter.php index 81b2333..76db5f3 100644 --- a/src/Php/Filter/UserDefinedSymbolFilter.php +++ b/src/Php/Filter/UserDefinedSymbolFilter.php @@ -6,11 +6,13 @@ namespace Mediact\DependencyGuard\Php\Filter; -use Roave\BetterReflection\Reflection\ReflectionClass; +use Mediact\DependencyGuard\Reflection\ReflectionTrait; use Throwable; class UserDefinedSymbolFilter implements SymbolFilterInterface { + use ReflectionTrait; + /** * Filter the given symbol. * @@ -21,7 +23,7 @@ class UserDefinedSymbolFilter implements SymbolFilterInterface public function __invoke(string $symbol): bool { try { - $reflection = ReflectionClass::createFromName($symbol); + $reflection = $this->getClassReflection($symbol); } catch (Throwable $e) { return false; } diff --git a/src/Php/SymbolExtractor.php b/src/Php/SymbolExtractor.php index 35dc497..e9d8fed 100644 --- a/src/Php/SymbolExtractor.php +++ b/src/Php/SymbolExtractor.php @@ -48,6 +48,8 @@ public function extract( $symbols = []; foreach ($files as $file) { + $parser = clone $this->parser; + try { $size = $file->getSize(); $handle = $file->openFile('rb'); @@ -57,7 +59,7 @@ public function extract( continue; } - $statements = $this->parser->parse($contents); + $statements = $parser->parse($contents); } catch (Error $e) { // Either not a file, file is not readable, not a PHP file or // the broken file should be detected by other tooling entirely. diff --git a/src/Reflection/ReflectionTrait.php b/src/Reflection/ReflectionTrait.php new file mode 100644 index 0000000..80d42ae --- /dev/null +++ b/src/Reflection/ReflectionTrait.php @@ -0,0 +1,44 @@ +classReflector() + ->reflect($className); + } +} diff --git a/tests/Reflection/ReflectionTraitTest.php b/tests/Reflection/ReflectionTraitTest.php new file mode 100644 index 0000000..c381b11 --- /dev/null +++ b/tests/Reflection/ReflectionTraitTest.php @@ -0,0 +1,33 @@ +getMockForTrait(ReflectionTrait::class); + + $this->assertInstanceOf( + ReflectionClass::class, + $subject->getClassReflection(__CLASS__) + ); + } +} diff --git a/tests/Regression/Issue25/Issue25Test.php b/tests/Regression/Issue25/Issue25Test.php index 1c554eb..70a599c 100644 --- a/tests/Regression/Issue25/Issue25Test.php +++ b/tests/Regression/Issue25/Issue25Test.php @@ -27,7 +27,7 @@ public function testNoOutOfMemoryError(): void [ PHP_BINARY, '-d', - 'memory_limit=512M', + 'memory_limit=15M', '../../../bin/dependency-guard' ], __DIR__ From 8ab87a3ec9b1f20096e44475b6b4b891650aa828 Mon Sep 17 00:00:00 2001 From: Jan-Marten de Boer Date: Thu, 11 Oct 2018 15:32:24 +0200 Subject: [PATCH 3/3] Increase memory limit for CI environment Increase the threshold at which the regression test fails to allow more memory consuming environments to run the test. --- tests/Regression/Issue25/Issue25Test.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/Regression/Issue25/Issue25Test.php b/tests/Regression/Issue25/Issue25Test.php index 70a599c..7760dd8 100644 --- a/tests/Regression/Issue25/Issue25Test.php +++ b/tests/Regression/Issue25/Issue25Test.php @@ -27,7 +27,7 @@ public function testNoOutOfMemoryError(): void [ PHP_BINARY, '-d', - 'memory_limit=15M', + 'memory_limit=25M', '../../../bin/dependency-guard' ], __DIR__