From 1ee426e4ac82c7afcf0bb7a33327870f0d7f78a9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 12 Sep 2016 07:31:53 -0700 Subject: [PATCH] Add a specialized cache for storing "has setup ever worked?" Summary: Ref T11613. In D16503/T11598 I refined the setup flow to improve messaging for early-stage setup issues, but failed to fully untangle things. We sometimes still try to access a cache which uses configuration before we build configuration, which causes an error. Instead, store "are we in flight / has setup ever worked?" in a separate cache which doesn't use the cache namespace. This stops us from trying to read config before building config. Test Plan: Hit bad extension error with a fake extension, got a proper setup help page: {F1812803} Solved the error, reloaded, broke things again, got a "friendly" page: {F1812805} Reviewers: chad Reviewed By: chad Maniphest Tasks: T11613 Differential Revision: https://secure.phabricator.com/D16542 --- src/applications/cache/PhabricatorCaches.php | 43 ++++++++++++++++--- .../config/check/PhabricatorSetupCheck.php | 6 ++- src/infrastructure/env/PhabricatorEnv.php | 16 +++---- 3 files changed, 51 insertions(+), 14 deletions(-) diff --git a/src/applications/cache/PhabricatorCaches.php b/src/applications/cache/PhabricatorCaches.php index e9baa299a2..c9bd304ccf 100644 --- a/src/applications/cache/PhabricatorCaches.php +++ b/src/applications/cache/PhabricatorCaches.php @@ -143,6 +143,39 @@ final class PhabricatorCaches extends Phobject { } +/* -( Server State Cache )------------------------------------------------- */ + + + /** + * Highly specialized cache for storing server process state. + * + * We use this cache to track initial steps in the setup phase, before + * configuration is loaded. + * + * This cache does NOT use the cache namespace (it must be accessed before + * we build configuration), and is global across all instances on the host. + * + * @return PhutilKeyValueCacheStack Best available server state cache stack. + * @task setup + */ + public static function getServerStateCache() { + static $cache; + if (!$cache) { + $caches = self::buildSetupCaches('phabricator-server'); + + // NOTE: We are NOT adding a cache namespace here! This cache is shared + // across all instances on the host. + + $caches = self::addProfilerToCaches($caches); + $cache = id(new PhutilKeyValueCacheStack()) + ->setCaches($caches); + + } + return $cache; + } + + + /* -( Setup Cache )-------------------------------------------------------- */ @@ -163,7 +196,7 @@ final class PhabricatorCaches extends Phobject { public static function getSetupCache() { static $cache; if (!$cache) { - $caches = self::buildSetupCaches(); + $caches = self::buildSetupCaches('phabricator-setup'); $cache = self::newStackFromCaches($caches); } return $cache; @@ -173,7 +206,7 @@ final class PhabricatorCaches extends Phobject { /** * @task setup */ - private static function buildSetupCaches() { + private static function buildSetupCaches($cache_name) { // If this is the CLI, just build a setup cache. if (php_sapi_name() == 'cli') { return array(); @@ -188,7 +221,7 @@ final class PhabricatorCaches extends Phobject { // If we don't have APC, build a poor approximation on disk. This is still // much better than nothing; some setup steps are quite slow. - $disk_path = self::getSetupCacheDiskCachePath(); + $disk_path = self::getSetupCacheDiskCachePath($cache_name); if ($disk_path) { $disk = new PhutilOnDiskKeyValueCache(); $disk->setCacheFile($disk_path); @@ -205,7 +238,7 @@ final class PhabricatorCaches extends Phobject { /** * @task setup */ - private static function getSetupCacheDiskCachePath() { + private static function getSetupCacheDiskCachePath($name) { // The difficulty here is in choosing a path which will change on server // restart (we MUST have this property), but as rarely as possible // otherwise (we desire this property to give the cache the best hit rate @@ -230,7 +263,7 @@ final class PhabricatorCaches extends Phobject { $tmp_dir = sys_get_temp_dir(); - $tmp_path = $tmp_dir.DIRECTORY_SEPARATOR.'phabricator-setup'; + $tmp_path = $tmp_dir.DIRECTORY_SEPARATOR.$name; if (!file_exists($tmp_path)) { @mkdir($tmp_path); } diff --git a/src/applications/config/check/PhabricatorSetupCheck.php b/src/applications/config/check/PhabricatorSetupCheck.php index 0c9888cd77..7947f5aa79 100644 --- a/src/applications/config/check/PhabricatorSetupCheck.php +++ b/src/applications/config/check/PhabricatorSetupCheck.php @@ -74,6 +74,9 @@ abstract class PhabricatorSetupCheck extends Phobject { $cache = PhabricatorCaches::getSetupCache(); $cache->setKey('phabricator.setup.issue-keys', $keys); + $server_cache = PhabricatorCaches::getServerStateCache(); + $server_cache->setKey('phabricator.in-flight', 1); + if ($update_database) { $db_cache = new PhabricatorKeyValueDatabaseCache(); try { @@ -204,7 +207,8 @@ abstract class PhabricatorSetupCheck extends Phobject { * @return bool True if we've made it through setup since the last restart. */ final public static function isInFlight() { - return (self::getOpenSetupIssueKeys() !== null); + $cache = PhabricatorCaches::getServerStateCache(); + return (bool)$cache->getKey('phabricator.in-flight'); } final public static function loadAllChecks() { diff --git a/src/infrastructure/env/PhabricatorEnv.php b/src/infrastructure/env/PhabricatorEnv.php index 7ea00957ff..2dfe7d9c62 100644 --- a/src/infrastructure/env/PhabricatorEnv.php +++ b/src/infrastructure/env/PhabricatorEnv.php @@ -315,14 +315,6 @@ final class PhabricatorEnv extends Phobject { * @task read */ public static function getEnvConfig($key) { - if (isset(self::$cache[$key])) { - return self::$cache[$key]; - } - - if (array_key_exists($key, self::$cache)) { - return self::$cache[$key]; - } - if (!self::$sourceStack) { throw new Exception( pht( @@ -331,6 +323,14 @@ final class PhabricatorEnv extends Phobject { $key)); } + if (isset(self::$cache[$key])) { + return self::$cache[$key]; + } + + if (array_key_exists($key, self::$cache)) { + return self::$cache[$key]; + } + $result = self::$sourceStack->getKeys(array($key)); if (array_key_exists($key, $result)) { self::$cache[$key] = $result[$key];