From ae2e73ce809ff1141c24d0e4cb2aa1de49bcfa96 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 17 Jul 2012 12:06:25 -0700 Subject: [PATCH] Add "stop on redirect" and "always profile" debugging options Summary: Currently, it's hard to debug performance issues on POST pages. Add flags to stop redirects and always collect profiles. Also fix an issue with "all" profiles. This feature is mostly just for profiling DarkConsole itself and is rarely used, I think it's been broken for some time. There's no way to get to it with the UI. NOTE: Some JS workflows don't stop on redirect because they use JS/AJAX redirects. Test Plan: Enabled options, browsed, got stopped on redirects and had profiles generated. Disabled options and verified redirects and profiles work normally. Reviewers: vrana, btrahan Reviewed By: vrana CC: aran Differential Revision: https://secure.phabricator.com/D2990 --- conf/default.conf.php | 17 ++++++ .../plugin/DarkConsoleXHProfPlugin.php | 4 +- .../xhprof/DarkConsoleXHProfPluginAPI.php | 59 +++++++++++++++---- .../response/AphrontRedirectResponse.php | 38 +++++++++++- src/aphront/writeguard/AphrontWriteGuard.php | 11 ++++ webroot/index.php | 4 +- 6 files changed, 115 insertions(+), 18 deletions(-) diff --git a/conf/default.conf.php b/conf/default.conf.php index 426d00f774..6b856ea69f 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -1199,4 +1199,21 @@ return array( 'style.monospace' => '10px "Menlo", "Consolas", "Monaco", monospace', +// -- Debugging ------------------------------------------------------------- // + + // Enable this to change HTTP redirects into normal pages with a link to the + // redirection target. For example, after you submit a form you'll get a page + // saying "normally, you'd be redirected...". This is useful to examine + // service or profiler information on write pathways, or debug redirects. It + // also makes the UX horrible for normal use, so you should enable it only + // when debugging. + // + // NOTE: This does not currently work for forms with Javascript "workflow", + // since the redirect happens in Javascript. + 'debug.stop-on-redirect' => false, + + // Enable this to always profile every page. This is very slow! You should + // only enable it when debugging. + 'debug.profile-every-request' => false, + ); diff --git a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php index e2a03d7804..0c38b60f99 100644 --- a/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php +++ b/src/aphront/console/plugin/DarkConsoleXHProfPlugin.php @@ -103,8 +103,8 @@ final class DarkConsoleXHProfPlugin extends DarkConsolePlugin { public function willShutdown() { - if (isset($_REQUEST['__profile__']) && - $_REQUEST['__profile__'] != 'all') { + if (DarkConsoleXHProfPluginAPI::isProfilerRequested() && + (DarkConsoleXHProfPluginAPI::isProfilerRequested() !== 'all')) { $this->xhprofID = DarkConsoleXHProfPluginAPI::stopProfiler(); } } diff --git a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php index 38bc063aed..8ab312c4df 100644 --- a/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php +++ b/src/aphront/console/plugin/xhprof/DarkConsoleXHProfPluginAPI.php @@ -29,6 +29,18 @@ final class DarkConsoleXHProfPluginAPI { return extension_loaded('xhprof'); } + public static function isProfilerRequested() { + if (!empty($_REQUEST['__profile__'])) { + return $_REQUEST['__profile__']; + } + + if (PhabricatorEnv::getEnvConfig('debug.profile-every-request')) { + return PhabricatorEnv::getEnvConfig('debug.profile-every-request'); + } + + return false; + } + public static function includeXHProfLib() { // TODO: this is incredibly stupid, but we may not have Phutil metamodule // stuff loaded yet so we can't just phutil_get_library_root() our way @@ -41,8 +53,9 @@ final class DarkConsoleXHProfPluginAPI { require_once $root.'/externals/xhprof/xhprof_lib.php'; } + public static function hookProfiler() { - if (empty($_REQUEST['__profile__'])) { + if (!self::isProfilerRequested()) { return; } @@ -71,17 +84,41 @@ final class DarkConsoleXHProfPluginAPI { $data = serialize($data); $file_class = 'PhabricatorFile'; - // Since these happen on GET we can't do guarded writes. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + // Since these happen on GET we can't do guarded writes. These also + // sometimes happen after we've disposed of the write guard; in this + // case we need to disable the whole mechanism. - $file = call_user_func( - array($file_class, 'newFromFileData'), - $data, - array( - 'mime-type' => 'application/xhprof', - 'name' => 'profile.xhprof', - )); - return $file->getPHID(); + $use_scope = AphrontWriteGuard::isGuardActive(); + if ($use_scope) { + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(true); + } + + $caught = null; + try { + $file = call_user_func( + array($file_class, 'newFromFileData'), + $data, + array( + 'mime-type' => 'application/xhprof', + 'name' => 'profile.xhprof', + )); + } catch (Exception $ex) { + $caught = $ex; + } + + if ($use_scope) { + unset($unguarded); + } else { + AphrontWriteGuard::allowDangerousUnguardedWrites(false); + } + + if ($caught) { + throw $caught; + } else { + return $file->getPHID(); + } } else { return null; } diff --git a/src/aphront/response/AphrontRedirectResponse.php b/src/aphront/response/AphrontRedirectResponse.php index f49681436b..7d060bb16b 100644 --- a/src/aphront/response/AphrontRedirectResponse.php +++ b/src/aphront/response/AphrontRedirectResponse.php @@ -34,15 +34,47 @@ class AphrontRedirectResponse extends AphrontResponse { return (string)$this->uri; } + public function shouldStopForDebugging() { + return PhabricatorEnv::getEnvConfig('debug.stop-on-redirect'); + } + public function getHeaders() { - $headers = array( - array('Location', $this->uri), - ); + $headers = array(); + if (!$this->shouldStopForDebugging()) { + $headers[] = array('Location', $this->uri); + } $headers = array_merge(parent::getHeaders(), $headers); return $headers; } public function buildResponseString() { + if ($this->shouldStopForDebugging()) { + $view = new PhabricatorStandardPageView(); + $view->setRequest($this->getRequest()); + $view->setApplicationName('Debug'); + $view->setTitle('Stopped on Redirect'); + + $error = new AphrontErrorView(); + $error->setSeverity(AphrontErrorView::SEVERITY_NOTICE); + $error->setTitle('Stopped on Redirect'); + + $link = phutil_render_tag( + 'a', + array( + 'href' => $this->getURI(), + ), + 'Continue to: '.phutil_escape_html($this->getURI())); + + $error->appendChild( + '

You were stopped here because debug.stop-on-redirect '. + 'is set in your configuration.

'. + '

'.$link.'

'); + + $view->appendChild($error); + + return $view->render(); + } + return ''; } diff --git a/src/aphront/writeguard/AphrontWriteGuard.php b/src/aphront/writeguard/AphrontWriteGuard.php index 38d6ce7ad2..e0a61284cf 100644 --- a/src/aphront/writeguard/AphrontWriteGuard.php +++ b/src/aphront/writeguard/AphrontWriteGuard.php @@ -108,6 +108,17 @@ final class AphrontWriteGuard { } + /** + * Determine if there is an active write guard. + * + * @return bool + * @task manage + */ + public static function isGuardActive() { + return (bool)self::$instance; + } + + /* -( Protecting Writes )-------------------------------------------------- */ diff --git a/webroot/index.php b/webroot/index.php index 1b5076134a..cfca18e57a 100644 --- a/webroot/index.php +++ b/webroot/index.php @@ -220,8 +220,8 @@ $headers = array_merge($headers, $response->getHeaders()); $sink->writeHeaders($headers); // TODO: This shouldn't be possible in a production-configured environment. -if (isset($_REQUEST['__profile__']) && - ($_REQUEST['__profile__'] == 'all')) { +if (DarkConsoleXHProfPluginAPI::isProfilerRequested() && + DarkConsoleXHProfPluginAPI::isProfilerRequested() === 'all') { $profile = DarkConsoleXHProfPluginAPI::stopProfiler(); $profile = '