Skip to content

Commit 62e4104

Browse files
author
epriestley
committed
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
1 parent 06367c4 commit 62e4104

File tree

11 files changed

+111
-54
lines changed

11 files changed

+111
-54
lines changed

‎src/__celerity_resource_map__.php

Lines changed: 42 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -397,7 +397,7 @@
397397
),
398398
'aphront-dialog-view-css' =>
399399
array(
400-
'uri' => '/res/1c0a5f75/rsrc/css/aphront/dialog-view.css',
400+
'uri' => '/res/531ebee9/rsrc/css/aphront/dialog-view.css',
401401
'type' => 'css',
402402
'requires' =>
403403
array(
@@ -760,7 +760,7 @@
760760
),
761761
'javelin-behavior-aphront-drag-and-drop-textarea' =>
762762
array(
763-
'uri' => '/res/43f964a7/rsrc/js/application/core/behavior-drag-and-drop-textarea.js',
763+
'uri' => '/res/76a52ae3/rsrc/js/application/core/behavior-drag-and-drop-textarea.js',
764764
'type' => 'js',
765765
'requires' =>
766766
array(
@@ -2399,7 +2399,7 @@
23992399
), array(
24002400
'packages' =>
24012401
array(
2402-
'c7d9f973' =>
2402+
'9ca700e9' =>
24032403
array(
24042404
'name' => 'core.pkg.css',
24052405
'symbols' =>
@@ -2424,7 +2424,7 @@
24242424
17 => 'aphront-pager-view-css',
24252425
18 => 'phabricator-transaction-view-css',
24262426
),
2427-
'uri' => '/res/pkg/c7d9f973/core.pkg.css',
2427+
'uri' => '/res/pkg/9ca700e9/core.pkg.css',
24282428
'type' => 'css',
24292429
),
24302430
'21d01ed8' =>
@@ -2470,7 +2470,7 @@
24702470
'uri' => '/res/pkg/216b9542/differential.pkg.css',
24712471
'type' => 'css',
24722472
),
2473-
'a3cabbba' =>
2473+
'f6a0c401' =>
24742474
array(
24752475
'name' => 'differential.pkg.js',
24762476
'symbols' =>
@@ -2493,7 +2493,7 @@
24932493
15 => 'javelin-behavior-differential-dropdown-menus',
24942494
16 => 'javelin-behavior-buoyant',
24952495
),
2496-
'uri' => '/res/pkg/a3cabbba/differential.pkg.js',
2496+
'uri' => '/res/pkg/f6a0c401/differential.pkg.js',
24972497
'type' => 'js',
24982498
),
24992499
'4ccda8a6' =>
@@ -2570,20 +2570,20 @@
25702570
'reverse' =>
25712571
array(
25722572
'aphront-attached-file-view-css' => '8db30c56',
2573-
'aphront-crumbs-view-css' => 'c7d9f973',
2574-
'aphront-dialog-view-css' => 'c7d9f973',
2575-
'aphront-form-view-css' => 'c7d9f973',
2573+
'aphront-crumbs-view-css' => '9ca700e9',
2574+
'aphront-dialog-view-css' => '9ca700e9',
2575+
'aphront-form-view-css' => '9ca700e9',
25762576
'aphront-headsup-action-list-view-css' => '216b9542',
2577-
'aphront-list-filter-view-css' => 'c7d9f973',
2578-
'aphront-pager-view-css' => 'c7d9f973',
2579-
'aphront-panel-view-css' => 'c7d9f973',
2580-
'aphront-side-nav-view-css' => 'c7d9f973',
2581-
'aphront-table-view-css' => 'c7d9f973',
2582-
'aphront-tokenizer-control-css' => 'c7d9f973',
2583-
'aphront-typeahead-control-css' => 'c7d9f973',
2577+
'aphront-list-filter-view-css' => '9ca700e9',
2578+
'aphront-pager-view-css' => '9ca700e9',
2579+
'aphront-panel-view-css' => '9ca700e9',
2580+
'aphront-side-nav-view-css' => '9ca700e9',
2581+
'aphront-table-view-css' => '9ca700e9',
2582+
'aphront-tokenizer-control-css' => '9ca700e9',
2583+
'aphront-typeahead-control-css' => '9ca700e9',
25842584
'differential-changeset-view-css' => '216b9542',
25852585
'differential-core-view-css' => '216b9542',
2586-
'differential-inline-comment-editor' => 'a3cabbba',
2586+
'differential-inline-comment-editor' => 'f6a0c401',
25872587
'differential-local-commits-view-css' => '216b9542',
25882588
'differential-revision-add-comment-css' => '216b9542',
25892589
'differential-revision-comment-css' => '216b9542',
@@ -2594,27 +2594,27 @@
25942594
'diffusion-commit-view-css' => '4ccda8a6',
25952595
'javelin-behavior' => '4fbae2af',
25962596
'javelin-behavior-aphront-basic-tokenizer' => '2af849fb',
2597-
'javelin-behavior-aphront-drag-and-drop' => 'a3cabbba',
2598-
'javelin-behavior-aphront-drag-and-drop-textarea' => 'a3cabbba',
2597+
'javelin-behavior-aphront-drag-and-drop' => 'f6a0c401',
2598+
'javelin-behavior-aphront-drag-and-drop-textarea' => 'f6a0c401',
25992599
'javelin-behavior-aphront-form-disable-on-submit' => '21d01ed8',
2600-
'javelin-behavior-buoyant' => 'a3cabbba',
2601-
'javelin-behavior-differential-accept-with-errors' => 'a3cabbba',
2602-
'javelin-behavior-differential-add-reviewers-and-ccs' => 'a3cabbba',
2603-
'javelin-behavior-differential-comment-jump' => 'a3cabbba',
2604-
'javelin-behavior-differential-diff-radios' => 'a3cabbba',
2605-
'javelin-behavior-differential-dropdown-menus' => 'a3cabbba',
2606-
'javelin-behavior-differential-edit-inline-comments' => 'a3cabbba',
2607-
'javelin-behavior-differential-feedback-preview' => 'a3cabbba',
2608-
'javelin-behavior-differential-keyboard-navigation' => 'a3cabbba',
2609-
'javelin-behavior-differential-populate' => 'a3cabbba',
2610-
'javelin-behavior-differential-show-more' => 'a3cabbba',
2600+
'javelin-behavior-buoyant' => 'f6a0c401',
2601+
'javelin-behavior-differential-accept-with-errors' => 'f6a0c401',
2602+
'javelin-behavior-differential-add-reviewers-and-ccs' => 'f6a0c401',
2603+
'javelin-behavior-differential-comment-jump' => 'f6a0c401',
2604+
'javelin-behavior-differential-diff-radios' => 'f6a0c401',
2605+
'javelin-behavior-differential-dropdown-menus' => 'f6a0c401',
2606+
'javelin-behavior-differential-edit-inline-comments' => 'f6a0c401',
2607+
'javelin-behavior-differential-feedback-preview' => 'f6a0c401',
2608+
'javelin-behavior-differential-keyboard-navigation' => 'f6a0c401',
2609+
'javelin-behavior-differential-populate' => 'f6a0c401',
2610+
'javelin-behavior-differential-show-more' => 'f6a0c401',
26112611
'javelin-behavior-maniphest-batch-selector' => '86fc0b0c',
26122612
'javelin-behavior-maniphest-transaction-controls' => '86fc0b0c',
26132613
'javelin-behavior-maniphest-transaction-expand' => '86fc0b0c',
26142614
'javelin-behavior-maniphest-transaction-preview' => '86fc0b0c',
26152615
'javelin-behavior-phabricator-autofocus' => '21d01ed8',
26162616
'javelin-behavior-phabricator-keyboard-shortcuts' => '21d01ed8',
2617-
'javelin-behavior-phabricator-object-selector' => 'a3cabbba',
2617+
'javelin-behavior-phabricator-object-selector' => 'f6a0c401',
26182618
'javelin-behavior-phabricator-watch-anchor' => '21d01ed8',
26192619
'javelin-behavior-refresh-csrf' => '21d01ed8',
26202620
'javelin-behavior-workflow' => '21d01ed8',
@@ -2637,23 +2637,23 @@
26372637
'javelin-workflow' => '21d01ed8',
26382638
'maniphest-task-summary-css' => '8db30c56',
26392639
'maniphest-transaction-detail-css' => '8db30c56',
2640-
'phabricator-app-buttons-css' => 'c7d9f973',
2640+
'phabricator-app-buttons-css' => '9ca700e9',
26412641
'phabricator-content-source-view-css' => '216b9542',
2642-
'phabricator-core-buttons-css' => 'c7d9f973',
2643-
'phabricator-core-css' => 'c7d9f973',
2644-
'phabricator-directory-css' => 'c7d9f973',
2645-
'phabricator-drag-and-drop-file-upload' => 'a3cabbba',
2642+
'phabricator-core-buttons-css' => '9ca700e9',
2643+
'phabricator-core-css' => '9ca700e9',
2644+
'phabricator-directory-css' => '9ca700e9',
2645+
'phabricator-drag-and-drop-file-upload' => 'f6a0c401',
26462646
'phabricator-dropdown-menu' => '21d01ed8',
2647-
'phabricator-jump-nav' => 'c7d9f973',
2647+
'phabricator-jump-nav' => '9ca700e9',
26482648
'phabricator-keyboard-shortcut' => '21d01ed8',
26492649
'phabricator-keyboard-shortcut-manager' => '21d01ed8',
26502650
'phabricator-menu-item' => '21d01ed8',
26512651
'phabricator-object-selector-css' => '216b9542',
26522652
'phabricator-paste-file-upload' => '21d01ed8',
2653-
'phabricator-remarkup-css' => 'c7d9f973',
2654-
'phabricator-shaped-request' => 'a3cabbba',
2655-
'phabricator-standard-page-view' => 'c7d9f973',
2656-
'phabricator-transaction-view-css' => 'c7d9f973',
2657-
'syntax-highlighting-css' => 'c7d9f973',
2653+
'phabricator-remarkup-css' => '9ca700e9',
2654+
'phabricator-shaped-request' => 'f6a0c401',
2655+
'phabricator-standard-page-view' => '9ca700e9',
2656+
'phabricator-transaction-view-css' => '9ca700e9',
2657+
'syntax-highlighting-css' => '9ca700e9',
26582658
),
26592659
));

‎src/__phutil_library_map__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -604,6 +604,7 @@
604604
'PhabricatorFileProxyImage' => 'applications/files/storage/proxyimage',
605605
'PhabricatorFileSideNavView' => 'applications/files/view/sidenav',
606606
'PhabricatorFileStorageBlob' => 'applications/files/storage/storageblob',
607+
'PhabricatorFileStorageConfigurationException' => 'applications/files/exception/configuration',
607608
'PhabricatorFileStorageEngine' => 'applications/files/engine/base',
608609
'PhabricatorFileStorageEngineSelector' => 'applications/files/engineselector/base',
609610
'PhabricatorFileTransformController' => 'applications/files/controller/transform',

‎src/applications/files/engine/localdisk/PhabricatorLocalDiskFileStorageEngine.php

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<?php
22

33
/*
4-
* Copyright 2011 Facebook, Inc.
4+
* Copyright 2012 Facebook, Inc.
55
*
66
* Licensed under the Apache License, Version 2.0 (the "License");
77
* you may not use this file except in compliance with the License.
@@ -109,7 +109,7 @@ private function getLocalDiskFileStorageRoot() {
109109
$root = PhabricatorEnv::getEnvConfig('storage.local-disk.path');
110110

111111
if (!$root || $root == '/' || $root[0] != '/') {
112-
throw new Exception(
112+
throw new PhabricatorFileStorageConfigurationException(
113113
"Malformed local disk storage root. You must provide an absolute ".
114114
"path, and can not use '/' as the root.");
115115
}

‎src/applications/files/engine/localdisk/__init__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
phutil_require_module('phabricator', 'aphront/writeguard');
1010
phutil_require_module('phabricator', 'applications/files/engine/base');
11+
phutil_require_module('phabricator', 'applications/files/exception/configuration');
1112
phutil_require_module('phabricator', 'infrastructure/env');
1213

1314
phutil_require_module('phutil', 'filesystem');

‎src/applications/files/engine/s3/PhabricatorS3FileStorageEngine.php

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
<?php
22

33
/*
4-
* Copyright 2011 Facebook, Inc.
4+
* Copyright 2012 Facebook, Inc.
55
*
66
* Licensed under the Apache License, Version 2.0 (the "License");
77
* you may not use this file except in compliance with the License.
@@ -96,7 +96,8 @@ public function deleteFile($handle) {
9696
private function getBucketName() {
9797
$bucket = PhabricatorEnv::getEnvConfig('storage.s3.bucket');
9898
if (!$bucket) {
99-
throw new Exception("No 'storage.s3.bucket' specified!");
99+
throw new PhabricatorFileStorageConfigurationException(
100+
"No 'storage.s3.bucket' specified!");
100101
}
101102
return $bucket;
102103
}
@@ -114,7 +115,7 @@ private function newS3API() {
114115
$secret_key = PhabricatorEnv::getEnvConfig('amazon-s3.secret-key');
115116

116117
if (!$access_key || !$secret_key) {
117-
throw new Exception(
118+
throw new PhabricatorFileStorageConfigurationException(
118119
"Specify 'amazon-s3.access-key' and 'amazon-s3.secret-key'!");
119120
}
120121

‎src/applications/files/engine/s3/__init__.php

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
phutil_require_module('phabricator', 'aphront/writeguard');
1010
phutil_require_module('phabricator', 'applications/files/engine/base');
11+
phutil_require_module('phabricator', 'applications/files/exception/configuration');
1112
phutil_require_module('phabricator', 'infrastructure/env');
1213

1314
phutil_require_module('phutil', 'filesystem');

‎src/applications/files/exception/configuration/PhabricatorFileStorageConfigurationException.php

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
<?php
2+
3+
/*
4+
* Copyright 2012 Facebook, Inc.
5+
*
6+
* Licensed under the Apache License, Version 2.0 (the "License");
7+
* you may not use this file except in compliance with the License.
8+
* You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing, software
13+
* distributed under the License is distributed on an "AS IS" BASIS,
14+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
15+
* See the License for the specific language governing permissions and
16+
* limitations under the License.
17+
*/
18+
19+
/**
20+
* Thrown by storage engines to indicate an configuration error which should
21+
* abort the storage attempt, as opposed to a transient storage error which
22+
* should be retried on other engines.
23+
*
24+
* @group files
25+
*/
26+
final class PhabricatorFileStorageConfigurationException extends Exception {
27+
28+
}

‎src/applications/files/exception/configuration/__init__.php

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?php
2+
/**
3+
* This file is automatically generated. Lint this module to rebuild it.
4+
* @generated
5+
*/
6+
7+
8+
9+
10+
phutil_require_source('PhabricatorFileStorageConfigurationException.php');

‎src/applications/files/storage/file/PhabricatorFile.php

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -93,22 +93,24 @@ public static function newFromFileData($data, array $params = array()) {
9393

9494
$data_handle = null;
9595
$engine_identifier = null;
96+
$exceptions = array();
9697
foreach ($engines as $engine) {
98+
$engine_class = get_class($engine);
9799
try {
98100
// Perform the actual write.
99101
$data_handle = $engine->writeFile($data, $params);
100102
if (!$data_handle || strlen($data_handle) > 255) {
101103
// This indicates an improperly implemented storage engine.
102-
throw new Exception(
103-
"Storage engine '{$engine}' executed writeFile() but did not ".
104-
"return a valid handle ('{$data_handle}') to the data: it must ".
105-
"be nonempty and no longer than 255 characters.");
104+
throw new PhabricatorFileStorageConfigurationException(
105+
"Storage engine '{$engine_class}' executed writeFile() but did ".
106+
"not return a valid handle ('{$data_handle}') to the data: it ".
107+
"must be nonempty and no longer than 255 characters.");
106108
}
107109

108110
$engine_identifier = $engine->getEngineIdentifier();
109111
if (!$engine_identifier || strlen($engine_identifier) > 32) {
110-
throw new Exception(
111-
"Storage engine '{$engine}' returned an improper engine ".
112+
throw new PhabricatorFileStorageConfigurationException(
113+
"Storage engine '{$engine_class}' returned an improper engine ".
112114
"identifier '{$engine_identifier}': it must be nonempty ".
113115
"and no longer than 32 characters.");
114116
}
@@ -117,14 +119,24 @@ public static function newFromFileData($data, array $params = array()) {
117119
// places.
118120
break;
119121
} catch (Exception $ex) {
122+
if ($ex instanceof PhabricatorFileStorageConfigurationException) {
123+
// If an engine is outright misconfigured (or misimplemented), raise
124+
// that immediately since it probably needs attention.
125+
throw $ex;
126+
}
127+
120128
// If an engine doesn't work, keep trying all the other valid engines
121129
// in case something else works.
122130
phlog($ex);
131+
132+
$exceptions[] = $ex;
123133
}
124134
}
125135

126136
if (!$data_handle) {
127-
throw new Exception("All storage engines failed to write file!");
137+
throw new PhutilAggregateException(
138+
"All storage engines failed to write file:",
139+
$exceptions);
128140
}
129141

130142
$file_name = idx($params, 'name');

‎src/applications/files/storage/file/__init__.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66

77

88

9+
phutil_require_module('phabricator', 'applications/files/exception/configuration');
910
phutil_require_module('phabricator', 'applications/files/exception/upload');
1011
phutil_require_module('phabricator', 'applications/files/storage/base');
1112
phutil_require_module('phabricator', 'applications/phid/constants');
@@ -14,6 +15,7 @@
1415
phutil_require_module('phabricator', 'infrastructure/util/hash');
1516

1617
phutil_require_module('phutil', 'error');
18+
phutil_require_module('phutil', 'error/aggregate');
1719
phutil_require_module('phutil', 'filesystem');
1820
phutil_require_module('phutil', 'filesystem/tempfile');
1921
phutil_require_module('phutil', 'markup');

0 commit comments

Comments
 (0)