From 62e41040f0bd0592fd25dbfc812fde74ff446fe0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 8 Apr 2012 15:07:34 -0700 Subject: [PATCH] Improve exception behavior for storage engine failures Summary: See T1021. Raise configuration or implementation exceptions immediately. When all engines fail, raise an aggregate exception with details. Test Plan: Forced all engines to fail, received an aggregate exception. Forced an engine to fail with a config exception, recevied it immediately. Reviewers: btrahan, vrana, jungejason Reviewed By: btrahan CC: aran Maniphest Tasks: T1021 Differential Revision: https://secure.phabricator.com/D2157 --- src/__celerity_resource_map__.php | 84 +++++++++---------- src/__phutil_library_map__.php | 1 + .../PhabricatorLocalDiskFileStorageEngine.php | 4 +- .../files/engine/localdisk/__init__.php | 1 + .../s3/PhabricatorS3FileStorageEngine.php | 7 +- src/applications/files/engine/s3/__init__.php | 1 + ...catorFileStorageConfigurationException.php | 28 +++++++ .../exception/configuration/__init__.php | 10 +++ .../files/storage/file/PhabricatorFile.php | 26 ++++-- .../files/storage/file/__init__.php | 2 + webroot/rsrc/css/aphront/dialog-view.css | 1 + 11 files changed, 111 insertions(+), 54 deletions(-) create mode 100644 src/applications/files/exception/configuration/PhabricatorFileStorageConfigurationException.php create mode 100644 src/applications/files/exception/configuration/__init__.php diff --git a/src/__celerity_resource_map__.php b/src/__celerity_resource_map__.php index 2a0564645c..ec82134bee 100644 --- a/src/__celerity_resource_map__.php +++ b/src/__celerity_resource_map__.php @@ -397,7 +397,7 @@ celerity_register_resource_map(array( ), 'aphront-dialog-view-css' => array( - 'uri' => '/res/1c0a5f75/rsrc/css/aphront/dialog-view.css', + 'uri' => '/res/531ebee9/rsrc/css/aphront/dialog-view.css', 'type' => 'css', 'requires' => array( @@ -760,7 +760,7 @@ celerity_register_resource_map(array( ), 'javelin-behavior-aphront-drag-and-drop-textarea' => array( - 'uri' => '/res/43f964a7/rsrc/js/application/core/behavior-drag-and-drop-textarea.js', + 'uri' => '/res/76a52ae3/rsrc/js/application/core/behavior-drag-and-drop-textarea.js', 'type' => 'js', 'requires' => array( @@ -2399,7 +2399,7 @@ celerity_register_resource_map(array( ), array( 'packages' => array( - 'c7d9f973' => + '9ca700e9' => array( 'name' => 'core.pkg.css', 'symbols' => @@ -2424,7 +2424,7 @@ celerity_register_resource_map(array( 17 => 'aphront-pager-view-css', 18 => 'phabricator-transaction-view-css', ), - 'uri' => '/res/pkg/c7d9f973/core.pkg.css', + 'uri' => '/res/pkg/9ca700e9/core.pkg.css', 'type' => 'css', ), '21d01ed8' => @@ -2470,7 +2470,7 @@ celerity_register_resource_map(array( 'uri' => '/res/pkg/216b9542/differential.pkg.css', 'type' => 'css', ), - 'a3cabbba' => + 'f6a0c401' => array( 'name' => 'differential.pkg.js', 'symbols' => @@ -2493,7 +2493,7 @@ celerity_register_resource_map(array( 15 => 'javelin-behavior-differential-dropdown-menus', 16 => 'javelin-behavior-buoyant', ), - 'uri' => '/res/pkg/a3cabbba/differential.pkg.js', + 'uri' => '/res/pkg/f6a0c401/differential.pkg.js', 'type' => 'js', ), '4ccda8a6' => @@ -2570,20 +2570,20 @@ celerity_register_resource_map(array( 'reverse' => array( 'aphront-attached-file-view-css' => '8db30c56', - 'aphront-crumbs-view-css' => 'c7d9f973', - 'aphront-dialog-view-css' => 'c7d9f973', - 'aphront-form-view-css' => 'c7d9f973', + 'aphront-crumbs-view-css' => '9ca700e9', + 'aphront-dialog-view-css' => '9ca700e9', + 'aphront-form-view-css' => '9ca700e9', 'aphront-headsup-action-list-view-css' => '216b9542', - 'aphront-list-filter-view-css' => 'c7d9f973', - 'aphront-pager-view-css' => 'c7d9f973', - 'aphront-panel-view-css' => 'c7d9f973', - 'aphront-side-nav-view-css' => 'c7d9f973', - 'aphront-table-view-css' => 'c7d9f973', - 'aphront-tokenizer-control-css' => 'c7d9f973', - 'aphront-typeahead-control-css' => 'c7d9f973', + 'aphront-list-filter-view-css' => '9ca700e9', + 'aphront-pager-view-css' => '9ca700e9', + 'aphront-panel-view-css' => '9ca700e9', + 'aphront-side-nav-view-css' => '9ca700e9', + 'aphront-table-view-css' => '9ca700e9', + 'aphront-tokenizer-control-css' => '9ca700e9', + 'aphront-typeahead-control-css' => '9ca700e9', 'differential-changeset-view-css' => '216b9542', 'differential-core-view-css' => '216b9542', - 'differential-inline-comment-editor' => 'a3cabbba', + 'differential-inline-comment-editor' => 'f6a0c401', 'differential-local-commits-view-css' => '216b9542', 'differential-revision-add-comment-css' => '216b9542', 'differential-revision-comment-css' => '216b9542', @@ -2594,27 +2594,27 @@ celerity_register_resource_map(array( 'diffusion-commit-view-css' => '4ccda8a6', 'javelin-behavior' => '4fbae2af', 'javelin-behavior-aphront-basic-tokenizer' => '2af849fb', - 'javelin-behavior-aphront-drag-and-drop' => 'a3cabbba', - 'javelin-behavior-aphront-drag-and-drop-textarea' => 'a3cabbba', + 'javelin-behavior-aphront-drag-and-drop' => 'f6a0c401', + 'javelin-behavior-aphront-drag-and-drop-textarea' => 'f6a0c401', 'javelin-behavior-aphront-form-disable-on-submit' => '21d01ed8', - 'javelin-behavior-buoyant' => 'a3cabbba', - 'javelin-behavior-differential-accept-with-errors' => 'a3cabbba', - 'javelin-behavior-differential-add-reviewers-and-ccs' => 'a3cabbba', - 'javelin-behavior-differential-comment-jump' => 'a3cabbba', - 'javelin-behavior-differential-diff-radios' => 'a3cabbba', - 'javelin-behavior-differential-dropdown-menus' => 'a3cabbba', - 'javelin-behavior-differential-edit-inline-comments' => 'a3cabbba', - 'javelin-behavior-differential-feedback-preview' => 'a3cabbba', - 'javelin-behavior-differential-keyboard-navigation' => 'a3cabbba', - 'javelin-behavior-differential-populate' => 'a3cabbba', - 'javelin-behavior-differential-show-more' => 'a3cabbba', + 'javelin-behavior-buoyant' => 'f6a0c401', + 'javelin-behavior-differential-accept-with-errors' => 'f6a0c401', + 'javelin-behavior-differential-add-reviewers-and-ccs' => 'f6a0c401', + 'javelin-behavior-differential-comment-jump' => 'f6a0c401', + 'javelin-behavior-differential-diff-radios' => 'f6a0c401', + 'javelin-behavior-differential-dropdown-menus' => 'f6a0c401', + 'javelin-behavior-differential-edit-inline-comments' => 'f6a0c401', + 'javelin-behavior-differential-feedback-preview' => 'f6a0c401', + 'javelin-behavior-differential-keyboard-navigation' => 'f6a0c401', + 'javelin-behavior-differential-populate' => 'f6a0c401', + 'javelin-behavior-differential-show-more' => 'f6a0c401', 'javelin-behavior-maniphest-batch-selector' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-controls' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-expand' => '86fc0b0c', 'javelin-behavior-maniphest-transaction-preview' => '86fc0b0c', 'javelin-behavior-phabricator-autofocus' => '21d01ed8', 'javelin-behavior-phabricator-keyboard-shortcuts' => '21d01ed8', - 'javelin-behavior-phabricator-object-selector' => 'a3cabbba', + 'javelin-behavior-phabricator-object-selector' => 'f6a0c401', 'javelin-behavior-phabricator-watch-anchor' => '21d01ed8', 'javelin-behavior-refresh-csrf' => '21d01ed8', 'javelin-behavior-workflow' => '21d01ed8', @@ -2637,23 +2637,23 @@ celerity_register_resource_map(array( 'javelin-workflow' => '21d01ed8', 'maniphest-task-summary-css' => '8db30c56', 'maniphest-transaction-detail-css' => '8db30c56', - 'phabricator-app-buttons-css' => 'c7d9f973', + 'phabricator-app-buttons-css' => '9ca700e9', 'phabricator-content-source-view-css' => '216b9542', - 'phabricator-core-buttons-css' => 'c7d9f973', - 'phabricator-core-css' => 'c7d9f973', - 'phabricator-directory-css' => 'c7d9f973', - 'phabricator-drag-and-drop-file-upload' => 'a3cabbba', + 'phabricator-core-buttons-css' => '9ca700e9', + 'phabricator-core-css' => '9ca700e9', + 'phabricator-directory-css' => '9ca700e9', + 'phabricator-drag-and-drop-file-upload' => 'f6a0c401', 'phabricator-dropdown-menu' => '21d01ed8', - 'phabricator-jump-nav' => 'c7d9f973', + 'phabricator-jump-nav' => '9ca700e9', 'phabricator-keyboard-shortcut' => '21d01ed8', 'phabricator-keyboard-shortcut-manager' => '21d01ed8', 'phabricator-menu-item' => '21d01ed8', 'phabricator-object-selector-css' => '216b9542', 'phabricator-paste-file-upload' => '21d01ed8', - 'phabricator-remarkup-css' => 'c7d9f973', - 'phabricator-shaped-request' => 'a3cabbba', - 'phabricator-standard-page-view' => 'c7d9f973', - 'phabricator-transaction-view-css' => 'c7d9f973', - 'syntax-highlighting-css' => 'c7d9f973', + 'phabricator-remarkup-css' => '9ca700e9', + 'phabricator-shaped-request' => 'f6a0c401', + 'phabricator-standard-page-view' => '9ca700e9', + 'phabricator-transaction-view-css' => '9ca700e9', + 'syntax-highlighting-css' => '9ca700e9', ), )); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e1b9a06f2b..3c1268fe87 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -604,6 +604,7 @@ phutil_register_library_map(array( 'PhabricatorFileProxyImage' => 'applications/files/storage/proxyimage', 'PhabricatorFileSideNavView' => 'applications/files/view/sidenav', 'PhabricatorFileStorageBlob' => 'applications/files/storage/storageblob', + 'PhabricatorFileStorageConfigurationException' => 'applications/files/exception/configuration', 'PhabricatorFileStorageEngine' => 'applications/files/engine/base', 'PhabricatorFileStorageEngineSelector' => 'applications/files/engineselector/base', 'PhabricatorFileTransformController' => 'applications/files/controller/transform', diff --git a/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php b/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php index 26bb03d481..d613d01673 100644 --- a/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php +++ b/src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php @@ -1,7 +1,7 @@ writeFile($data, $params); if (!$data_handle || strlen($data_handle) > 255) { // This indicates an improperly implemented storage engine. - throw new Exception( - "Storage engine '{$engine}' executed writeFile() but did not ". - "return a valid handle ('{$data_handle}') to the data: it must ". - "be nonempty and no longer than 255 characters."); + throw new PhabricatorFileStorageConfigurationException( + "Storage engine '{$engine_class}' executed writeFile() but did ". + "not return a valid handle ('{$data_handle}') to the data: it ". + "must be nonempty and no longer than 255 characters."); } $engine_identifier = $engine->getEngineIdentifier(); if (!$engine_identifier || strlen($engine_identifier) > 32) { - throw new Exception( - "Storage engine '{$engine}' returned an improper engine ". + throw new PhabricatorFileStorageConfigurationException( + "Storage engine '{$engine_class}' returned an improper engine ". "identifier '{$engine_identifier}': it must be nonempty ". "and no longer than 32 characters."); } @@ -117,14 +119,24 @@ final class PhabricatorFile extends PhabricatorFileDAO { // places. break; } catch (Exception $ex) { + if ($ex instanceof PhabricatorFileStorageConfigurationException) { + // If an engine is outright misconfigured (or misimplemented), raise + // that immediately since it probably needs attention. + throw $ex; + } + // If an engine doesn't work, keep trying all the other valid engines // in case something else works. phlog($ex); + + $exceptions[] = $ex; } } if (!$data_handle) { - throw new Exception("All storage engines failed to write file!"); + throw new PhutilAggregateException( + "All storage engines failed to write file:", + $exceptions); } $file_name = idx($params, 'name'); diff --git a/src/applications/files/storage/file/__init__.php b/src/applications/files/storage/file/__init__.php index 4de0139077..e72e0f6ae6 100644 --- a/src/applications/files/storage/file/__init__.php +++ b/src/applications/files/storage/file/__init__.php @@ -6,6 +6,7 @@ +phutil_require_module('phabricator', 'applications/files/exception/configuration'); phutil_require_module('phabricator', 'applications/files/exception/upload'); phutil_require_module('phabricator', 'applications/files/storage/base'); phutil_require_module('phabricator', 'applications/phid/constants'); @@ -14,6 +15,7 @@ phutil_require_module('phabricator', 'infrastructure/env'); phutil_require_module('phabricator', 'infrastructure/util/hash'); phutil_require_module('phutil', 'error'); +phutil_require_module('phutil', 'error/aggregate'); phutil_require_module('phutil', 'filesystem'); phutil_require_module('phutil', 'filesystem/tempfile'); phutil_require_module('phutil', 'markup'); diff --git a/webroot/rsrc/css/aphront/dialog-view.css b/webroot/rsrc/css/aphront/dialog-view.css index 142cbb7512..d17fd998b4 100644 --- a/webroot/rsrc/css/aphront/dialog-view.css +++ b/webroot/rsrc/css/aphront/dialog-view.css @@ -74,6 +74,7 @@ font-size: 14px; background: #efefef; padding: 1em; + white-space: pre-wrap; } .aphront-exception-dialog .exception-trace {