From 8da5f5eb1aa526aacc00982ac533f534a267508f Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Tue, 16 Aug 2022 15:48:49 +0200 Subject: [PATCH 1/5] use exceptions instead of error_log for ParserCacheProvider * use exceptions instead of error_log for ParserCacheProvider like all other cache providers do * remove duplicate code in ParserCacheProvider * use same hash as other cache providers * update Config.php cache directory creation to use same code as ParserCacheProvider --- src/Psalm/Config.php | 17 +- .../Internal/Provider/ParserCacheProvider.php | 184 ++++++++---------- .../Provider/ParserInstanceCacheProvider.php | 12 ++ 3 files changed, 109 insertions(+), 104 deletions(-) diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index 6dd9b63d125..534c8252906 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -35,6 +35,7 @@ use Psalm\Plugin\PluginEntryPointInterface; use Psalm\Progress\Progress; use Psalm\Progress\VoidProgress; +use RuntimeException; use SimpleXMLElement; use SimpleXMLIterator; use Throwable; @@ -55,7 +56,6 @@ use function clearstatcache; use function count; use function dirname; -use function error_log; use function explode; use function extension_loaded; use function file_exists; @@ -1013,8 +1013,19 @@ private static function fromXmlAndPaths( chdir($config->base_dir); } - if (is_dir($config->cache_directory) === false && @mkdir($config->cache_directory, 0777, true) === false) { - error_log('Could not create cache directory: ' . $config->cache_directory); + if (!is_dir($config->cache_directory)) { + try { + if (mkdir($config->cache_directory, 0777, true) === false) { + // any other error than directory already exists/permissions issue + throw new RuntimeException('Failed to create Psalm cache directory for unknown reasons'); + } + } catch (RuntimeException $e) { + if (!is_dir($config->cache_directory)) { + // rethrow the error with default message + // it contains the reason why creation failed + throw $e; + } + } } if ($cwd) { diff --git a/src/Psalm/Internal/Provider/ParserCacheProvider.php b/src/Psalm/Internal/Provider/ParserCacheProvider.php index c1c30c27b76..dc36c13f54c 100644 --- a/src/Psalm/Internal/Provider/ParserCacheProvider.php +++ b/src/Psalm/Internal/Provider/ParserCacheProvider.php @@ -7,12 +7,13 @@ use Psalm\Config; use Psalm\Internal\Provider\Providers; use RuntimeException; +use UnexpectedValueException; use function clearstatcache; -use function error_log; use function file_put_contents; use function filemtime; use function gettype; +use function hash; use function igbinary_serialize; use function igbinary_unserialize; use function is_array; @@ -21,7 +22,6 @@ use function is_writable; use function json_decode; use function json_encode; -use function md5; use function mkdir; use function scandir; use function serialize; @@ -42,31 +42,33 @@ class ParserCacheProvider private const PARSER_CACHE_DIRECTORY = 'php-parser'; private const FILE_CONTENTS_CACHE_DIRECTORY = 'file-caches'; + /** + * @var Config + */ + private $config; + /** * A map of filename hashes to contents hashes * * @var array|null */ - private $existing_file_content_hashes; + protected $existing_file_content_hashes; /** * A map of recently-added filename hashes to contents hashes * * @var array */ - private $new_file_content_hashes = []; + protected $new_file_content_hashes = []; /** * @var bool */ private $use_file_cache; - /** @var bool */ - private $use_igbinary; - public function __construct(Config $config, bool $use_file_cache = true) { - $this->use_igbinary = $config->use_igbinary; + $this->config = $config; $this->use_file_cache = $use_file_cache; } @@ -78,28 +80,22 @@ public function loadStatementsFromCache( int $file_modified_time, string $file_content_hash ): ?array { - $root_cache_directory = Config::getInstance()->getCacheDirectory(); - - if (!$root_cache_directory) { + if (!$this->use_file_cache) { return null; } - $file_cache_key = $this->getParserCacheKey( - $file_path - ); + $cache_location = $this->getCacheLocationForPath($file_path, self::PARSER_CACHE_DIRECTORY); - $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::PARSER_CACHE_DIRECTORY; + $file_cache_key = $this->getParserCacheKey($file_path); $file_content_hashes = $this->new_file_content_hashes + $this->getExistingFileContentHashes(); - $cache_location = $parser_cache_directory . DIRECTORY_SEPARATOR . $file_cache_key; - if (isset($file_content_hashes[$file_cache_key]) && $file_content_hash === $file_content_hashes[$file_cache_key] && is_readable($cache_location) && filemtime($cache_location) > $file_modified_time ) { - if ($this->use_igbinary) { + if ($this->config->use_igbinary) { /** @var list */ $stmts = igbinary_unserialize(Providers::safeFileGetContents($cache_location)); } else { @@ -118,22 +114,14 @@ public function loadStatementsFromCache( */ public function loadExistingStatementsFromCache(string $file_path): ?array { - $root_cache_directory = Config::getInstance()->getCacheDirectory(); - - if (!$root_cache_directory) { + if (!$this->use_file_cache) { return null; } - $file_cache_key = $this->getParserCacheKey( - $file_path - ); - - $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::PARSER_CACHE_DIRECTORY; - - $cache_location = $parser_cache_directory . DIRECTORY_SEPARATOR . $file_cache_key; + $cache_location = $this->getCacheLocationForPath($file_path, self::PARSER_CACHE_DIRECTORY); if (is_readable($cache_location)) { - if ($this->use_igbinary) { + if ($this->config->use_igbinary) { /** @var list */ return igbinary_unserialize(Providers::safeFileGetContents($cache_location)) ?: null; } @@ -151,19 +139,7 @@ public function loadExistingFileContentsFromCache(string $file_path): ?string return null; } - $root_cache_directory = Config::getInstance()->getCacheDirectory(); - - if (!$root_cache_directory) { - return null; - } - - $file_cache_key = $this->getParserCacheKey( - $file_path - ); - - $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::FILE_CONTENTS_CACHE_DIRECTORY; - - $cache_location = $parser_cache_directory . DIRECTORY_SEPARATOR . $file_cache_key; + $cache_location = $this->getCacheLocationForPath($file_path, self::FILE_CONTENTS_CACHE_DIRECTORY); if (is_readable($cache_location)) { return Providers::safeFileGetContents($cache_location); @@ -177,35 +153,37 @@ public function loadExistingFileContentsFromCache(string $file_path): ?string */ private function getExistingFileContentHashes(): array { - $config = Config::getInstance(); - $root_cache_directory = $config->getCacheDirectory(); + if (!$this->use_file_cache) { + return []; + } if ($this->existing_file_content_hashes === null) { + $root_cache_directory = $this->config->getCacheDirectory(); $file_hashes_path = $root_cache_directory . DIRECTORY_SEPARATOR . self::FILE_HASHES; - if ($root_cache_directory && is_readable($file_hashes_path)) { - $hashes_encoded = Providers::safeFileGetContents($file_hashes_path); - if (!$hashes_encoded) { - error_log('Unexpected value when loading from file content hashes'); - $this->existing_file_content_hashes = []; - - return []; - } + if (!$root_cache_directory) { + throw new UnexpectedValueException('No cache directory defined'); + } - $hashes_decoded = json_decode($hashes_encoded, true); + if (!is_readable($file_hashes_path)) { + // might not exist yet + $this->existing_file_content_hashes = []; + return $this->existing_file_content_hashes; + } - if (!is_array($hashes_decoded)) { - error_log('Unexpected value ' . gettype($hashes_decoded)); - $this->existing_file_content_hashes = []; + $hashes_encoded = Providers::safeFileGetContents($file_hashes_path); + if (!$hashes_encoded) { + throw new UnexpectedValueException('File content hashes should be in cache'); + } - return []; - } + $hashes_decoded = json_decode($hashes_encoded, true); - /** @var array $hashes_decoded */ - $this->existing_file_content_hashes = $hashes_decoded; - } else { - $this->existing_file_content_hashes = []; + if (!is_array($hashes_decoded)) { + throw new UnexpectedValueException('File content hashes are of invalid type ' . gettype($hashes_decoded)); } + + /** @var array $hashes_decoded */ + $this->existing_file_content_hashes = $hashes_decoded; } return $this->existing_file_content_hashes; @@ -220,31 +198,18 @@ public function saveStatementsToCache( array $stmts, bool $touch_only ): void { - $root_cache_directory = Config::getInstance()->getCacheDirectory(); - - if (!$root_cache_directory) { - return; - } - - $file_cache_key = $this->getParserCacheKey( - $file_path - ); - - $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::PARSER_CACHE_DIRECTORY; - - $cache_location = $parser_cache_directory . DIRECTORY_SEPARATOR . $file_cache_key; + $cache_location = $this->getCacheLocationForPath($file_path, self::PARSER_CACHE_DIRECTORY, !$touch_only); if ($touch_only) { touch($cache_location); } else { - $this->createCacheDirectory($parser_cache_directory); - - if ($this->use_igbinary) { + if ($this->config->use_igbinary) { file_put_contents($cache_location, igbinary_serialize($stmts), LOCK_EX); } else { file_put_contents($cache_location, serialize($stmts), LOCK_EX); } + $file_cache_key = $this->getParserCacheKey($file_path); $this->new_file_content_hashes[$file_cache_key] = $file_content_hash; } } @@ -268,7 +233,11 @@ public function addNewFileContentHashes(array $file_content_hashes): void public function saveFileContentHashes(): void { - $root_cache_directory = Config::getInstance()->getCacheDirectory(); + if (!$this->use_file_cache) { + return; + } + + $root_cache_directory = $this->config->getCacheDirectory(); if (!$root_cache_directory) { return; @@ -298,28 +267,17 @@ public function cacheFileContents(string $file_path, string $file_contents): voi return; } - $root_cache_directory = Config::getInstance()->getCacheDirectory(); - - if (!$root_cache_directory) { - return; - } - - $file_cache_key = $this->getParserCacheKey( - $file_path - ); - - $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::FILE_CONTENTS_CACHE_DIRECTORY; - - $cache_location = $parser_cache_directory . DIRECTORY_SEPARATOR . $file_cache_key; - - $this->createCacheDirectory($parser_cache_directory); + $cache_location = $this->getCacheLocationForPath($file_path, self::FILE_CONTENTS_CACHE_DIRECTORY, true); file_put_contents($cache_location, $file_contents, LOCK_EX); } public function deleteOldParserCaches(float $time_before): int { - $cache_directory = Config::getInstance()->getCacheDirectory(); + $cache_directory = $this->config->getCacheDirectory(); + + $this->existing_file_content_hashes = null; + $this->new_file_content_hashes = []; if (!$cache_directory) { return 0; @@ -349,22 +307,46 @@ public function deleteOldParserCaches(float $time_before): int return $removed_count; } - private function getParserCacheKey(string $file_name): string + private function getParserCacheKey(string $file_path): string { - return md5($file_name) . ($this->use_igbinary ? '-igbinary' : '') . '-r'; + if (PHP_VERSION_ID >= 80100) { + $hash = hash('xxh128', $file_path); + } else { + $hash = hash('md4', $file_path); + } + + return $hash . ($this->config->use_igbinary ? '-igbinary' : '') . '-r'; } - private function createCacheDirectory(string $parser_cache_directory): void + + private function getCacheLocationForPath(string $file_path, string $subdirectory, bool $create_directory = false): string { - if (!is_dir($parser_cache_directory)) { + $root_cache_directory = $this->config->getCacheDirectory(); + + if (!$root_cache_directory) { + throw new UnexpectedValueException('No cache directory defined'); + } + + $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . $subdirectory; + + if ($create_directory && !is_dir($parser_cache_directory)) { try { - mkdir($parser_cache_directory, 0777, true); + if (mkdir($parser_cache_directory, 0777, true) === false) { + // any other error than directory already exists/permissions issue + throw new RuntimeException('Failed to create ' . $parser_cache_directory . ' cache directory for unknown reasons'); + } } catch (RuntimeException $e) { // Race condition (#4483) if (!is_dir($parser_cache_directory)) { - error_log('Could not create parser cache directory: ' . $parser_cache_directory); + // rethrow the error with default message + // it contains the reason why creation failed + throw $e; } } } + + return $parser_cache_directory + . DIRECTORY_SEPARATOR + . $this->getParserCacheKey($file_path); } } diff --git a/tests/Internal/Provider/ParserInstanceCacheProvider.php b/tests/Internal/Provider/ParserInstanceCacheProvider.php index 9b81bfcef87..766772cd600 100644 --- a/tests/Internal/Provider/ParserInstanceCacheProvider.php +++ b/tests/Internal/Provider/ParserInstanceCacheProvider.php @@ -82,6 +82,18 @@ public function cacheFileContents(string $file_path, string $file_contents): voi $this->file_contents_cache[$file_path] = $file_contents; } + public function deleteOldParserCaches(float $time_before): int + { + $this->existing_file_content_hashes = null; + $this->new_file_content_hashes = []; + + $this->file_contents_cache = []; + $this->file_content_hash = []; + $this->statements_cache = []; + $this->statements_cache_time = []; + return 0; + } + public function saveFileContentHashes(): void { } From 4726454f49095bdfca0e024f850af079362927a8 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Tue, 16 Aug 2022 15:53:28 +0200 Subject: [PATCH 2/5] update leftover md5 in provider to commonly used hash Revert "update leftover md5 in provider to commonly used hash" This reverts commit 66337ecf50446dca8650a0812ebfe516d1993e06. partially put back Update StatementsProvider.php --- src/Psalm/Internal/Provider/StatementsProvider.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/Psalm/Internal/Provider/StatementsProvider.php b/src/Psalm/Internal/Provider/StatementsProvider.php index 431cb1585d4..75e47809b6e 100644 --- a/src/Psalm/Internal/Provider/StatementsProvider.php +++ b/src/Psalm/Internal/Provider/StatementsProvider.php @@ -26,6 +26,7 @@ use function array_merge; use function count; use function filemtime; +use function hash; use function md5; use function strlen; use function strpos; @@ -132,7 +133,11 @@ public function getStatementsForFile(string $file_path, string $php_version, ?Pr $config = Config::getInstance(); - $file_content_hash = md5($version . $file_contents); + if (PHP_VERSION_ID >= 80100) { + $file_content_hash = hash('xxh128', $version . $file_contents); + } else { + $file_content_hash = hash('md4', $version . $file_contents); + } if (!$this->parser_cache_provider || (!$config->isInProjectDirs($file_path) && strpos($file_path, 'vendor')) @@ -239,6 +244,7 @@ function (int $_): bool { array_flip($unchanged_signature_members) ); + // do NOT change this to hash, it will fail on Windows for whatever reason $file_path_hash = md5($file_path); $changed_members = array_map( From 8ac86f0a4d99d65ee8932c0a74e79d2b6a5c2f9a Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Thu, 18 Aug 2022 13:39:46 +0200 Subject: [PATCH 3/5] use consistent race condition dir creation code in all places in cache --- .../ClassLikeStorageCacheProvider.php | 15 +++++++- .../Provider/FileReferenceCacheProvider.php | 36 +++++++++++++------ .../Provider/FileStorageCacheProvider.php | 15 +++++++- 3 files changed, 54 insertions(+), 12 deletions(-) diff --git a/src/Psalm/Internal/Provider/ClassLikeStorageCacheProvider.php b/src/Psalm/Internal/Provider/ClassLikeStorageCacheProvider.php index c7fa19acd0c..a6aa8e7b129 100644 --- a/src/Psalm/Internal/Provider/ClassLikeStorageCacheProvider.php +++ b/src/Psalm/Internal/Provider/ClassLikeStorageCacheProvider.php @@ -5,6 +5,7 @@ use Psalm\Config; use Psalm\Internal\Provider\Providers; use Psalm\Storage\ClassLikeStorage; +use RuntimeException; use UnexpectedValueException; use function array_merge; @@ -168,7 +169,19 @@ private function getCacheLocationForClass( $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::CLASS_CACHE_DIRECTORY; if ($create_directory && !is_dir($parser_cache_directory)) { - mkdir($parser_cache_directory, 0777, true); + try { + if (mkdir($parser_cache_directory, 0777, true) === false) { + // any other error than directory already exists/permissions issue + throw new RuntimeException('Failed to create ' . $parser_cache_directory . ' cache directory for unknown reasons'); + } + } catch (RuntimeException $e) { + // Race condition (#4483) + if (!is_dir($parser_cache_directory)) { + // rethrow the error with default message + // it contains the reason why creation failed + throw $e; + } + } } $data = $file_path ? strtolower($file_path) . ' ' : ''; diff --git a/src/Psalm/Internal/Provider/FileReferenceCacheProvider.php b/src/Psalm/Internal/Provider/FileReferenceCacheProvider.php index e512aacf29b..76f12afb09f 100644 --- a/src/Psalm/Internal/Provider/FileReferenceCacheProvider.php +++ b/src/Psalm/Internal/Provider/FileReferenceCacheProvider.php @@ -5,6 +5,7 @@ use Psalm\Config; use Psalm\Internal\Codebase\Analyzer; use Psalm\Internal\Provider\Providers; +use RuntimeException; use UnexpectedValueException; use function file_exists; @@ -12,6 +13,7 @@ use function igbinary_serialize; use function igbinary_unserialize; use function is_array; +use function is_dir; use function is_readable; use function mkdir; use function serialize; @@ -992,18 +994,32 @@ public function setConfigHashCache(string $hash): void { $cache_directory = Config::getInstance()->getCacheDirectory(); - if ($cache_directory) { - if (!file_exists($cache_directory)) { - mkdir($cache_directory, 0777, true); + if (!$cache_directory) { + return; + } + + if (!is_dir($cache_directory)) { + try { + if (mkdir($cache_directory, 0777, true) === false) { + // any other error than directory already exists/permissions issue + throw new RuntimeException('Failed to create ' . $cache_directory . ' cache directory for unknown reasons'); + } + } catch (RuntimeException $e) { + // Race condition (#4483) + if (!is_dir($cache_directory)) { + // rethrow the error with default message + // it contains the reason why creation failed + throw $e; + } } + } - $config_hash_cache_location = $cache_directory . DIRECTORY_SEPARATOR . self::CONFIG_HASH_CACHE_NAME; + $config_hash_cache_location = $cache_directory . DIRECTORY_SEPARATOR . self::CONFIG_HASH_CACHE_NAME; - file_put_contents( - $config_hash_cache_location, - $hash, - LOCK_EX - ); - } + file_put_contents( + $config_hash_cache_location, + $hash, + LOCK_EX + ); } } diff --git a/src/Psalm/Internal/Provider/FileStorageCacheProvider.php b/src/Psalm/Internal/Provider/FileStorageCacheProvider.php index 2f9279f12ae..ddcbf22a633 100644 --- a/src/Psalm/Internal/Provider/FileStorageCacheProvider.php +++ b/src/Psalm/Internal/Provider/FileStorageCacheProvider.php @@ -5,6 +5,7 @@ use Psalm\Config; use Psalm\Internal\Provider\Providers; use Psalm\Storage\FileStorage; +use RuntimeException; use UnexpectedValueException; use function array_merge; @@ -168,7 +169,19 @@ private function getCacheLocationForPath(string $file_path, bool $create_directo $parser_cache_directory = $root_cache_directory . DIRECTORY_SEPARATOR . self::FILE_STORAGE_CACHE_DIRECTORY; if ($create_directory && !is_dir($parser_cache_directory)) { - mkdir($parser_cache_directory, 0777, true); + try { + if (mkdir($parser_cache_directory, 0777, true) === false) { + // any other error than directory already exists/permissions issue + throw new RuntimeException('Failed to create ' . $parser_cache_directory . ' cache directory for unknown reasons'); + } + } catch (RuntimeException $e) { + // Race condition (#4483) + if (!is_dir($parser_cache_directory)) { + // rethrow the error with default message + // it contains the reason why creation failed + throw $e; + } + } } if (PHP_VERSION_ID >= 80100) { From 978f37e421a1c4a96cc0b9b0445deca97ea443aa Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Wed, 24 Aug 2022 12:18:46 +0200 Subject: [PATCH 4/5] improve unlinking potential race condition * fix rare race condition on file cache unlink * remove unnecessary reset() * improve code readability using variable --- src/Psalm/Config.php | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/Psalm/Config.php b/src/Psalm/Config.php index 534c8252906..380e8399751 100644 --- a/src/Psalm/Config.php +++ b/src/Psalm/Config.php @@ -2298,20 +2298,29 @@ public static function removeCacheDirectory(string $dir): void continue; } + $full_path = $dir . '/' . $object; + // if it was deleted in the meantime/race condition with other psalm process - if (!file_exists($dir . '/' . $object)) { + if (!file_exists($full_path)) { continue; } - if (filetype($dir . '/' . $object) === 'dir') { - self::removeCacheDirectory($dir . '/' . $object); + if (filetype($full_path) === 'dir') { + self::removeCacheDirectory($full_path); } else { - unlink($dir . '/' . $object); + try { + unlink($full_path); + } catch (RuntimeException $e) { + clearstatcache(true, $full_path); + if (file_exists($full_path)) { + // rethrow the error with default message + // it contains the reason why deletion failed + throw $e; + } + } } } - reset($objects); - // may have been removed in the meantime clearstatcache(true, $dir); if (is_dir($dir)) { From 62df25a741a3a5f24c373e7ee8f7b66bc7e954b7 Mon Sep 17 00:00:00 2001 From: kkmuffme <11071985+kkmuffme@users.noreply.github.com> Date: Thu, 8 Sep 2022 23:57:12 +0200 Subject: [PATCH 5/5] fix test cache inconsistency --- tests/Internal/Provider/FakeParserCacheProvider.php | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/Internal/Provider/FakeParserCacheProvider.php b/tests/Internal/Provider/FakeParserCacheProvider.php index 06b31afd84b..36b1dbb3d50 100644 --- a/tests/Internal/Provider/FakeParserCacheProvider.php +++ b/tests/Internal/Provider/FakeParserCacheProvider.php @@ -33,6 +33,14 @@ public function cacheFileContents(string $file_path, string $file_contents): voi { } + public function deleteOldParserCaches(float $time_before): int + { + $this->existing_file_content_hashes = null; + $this->new_file_content_hashes = []; + + return 0; + } + public function saveFileContentHashes(): void { }