From 7db265cd5da532e8c3f1936702ce7fc0e5ee28a4 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Sep 2018 08:37:26 -0700 Subject: [PATCH 1/7] Parameterize the repository read and write locks Summary: Ref T13202. See PHI889. Update the read and write locks to the modern parameterized verison, which handles hashing/normalization and can store better logs. This parameterized mode was added in D19173 and has been used successfully for some time, but not all locks have switched over to it yet. Test Plan: - Added an `fprintf(STDERR, $full_name)` to the lock code. - Pulled a repository. - Saw sensible lock name on stdout before "acquired read lock...". - Additional changes in this patch series will vet this more completely. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19701 --- .../PhabricatorRepositoryWorkingCopyVersion.php | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php index da5d54b57d..e297dfbf0e 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php +++ b/src/applications/repository/storage/PhabricatorRepositoryWorkingCopyVersion.php @@ -76,18 +76,20 @@ final class PhabricatorRepositoryWorkingCopyVersion public static function getReadLock($repository_phid, $device_phid) { - $repository_hash = PhabricatorHash::digestForIndex($repository_phid); - $device_hash = PhabricatorHash::digestForIndex($device_phid); - $lock_key = "repo.read({$repository_hash}, {$device_hash})"; + $parameters = array( + 'repositoryPHID' => $repository_phid, + 'devicePHID' => $device_phid, + ); - return PhabricatorGlobalLock::newLock($lock_key); + return PhabricatorGlobalLock::newLock('repo.read', $parameters); } public static function getWriteLock($repository_phid) { - $repository_hash = PhabricatorHash::digestForIndex($repository_phid); - $lock_key = "repo.write({$repository_hash})"; + $parameters = array( + 'repositoryPHID' => $repository_phid, + ); - return PhabricatorGlobalLock::newLock($lock_key); + return PhabricatorGlobalLock::newLock('repo.write', $parameters); } From 021c612cb228ad29fb1d4d7f63276cf29bef754a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 23 Sep 2018 11:01:29 -0700 Subject: [PATCH 2/7] When we fail to acquire a repository lock, try to provide a hint about why Summary: Ref T13202. See PHI889. If the lock log is enabled, we can try to offer more details about lock holders. When we fail to acquire a lock: - check for recent acquisitions and suggest that this is a bottleneck issue; - if there are no recent acquisitions, check for the last acquisition and print details about it (what process, how long ago, whether or not we believe it was released). Test Plan: - Enabled the lock log. - Changed the lock wait time to 1 second. - Added a `sleep(10)` after grabbing the lock. - In one window, ran a Conduit call or a `git fetch`. - In another window, ran another operation. - Got useful/sensible errors for both ssh and web lock holders, for example: > PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=12609, host=orbital-3.local, sapi=apache2handler, controller=PhabricatorConduitAPIController, method=diffusion.rawdiffquery) 3 second(s) ago. There is no record of this lock being released.) > PhutilProxyException: Failed to acquire read lock after waiting 1 second(s). You may be able to retry later. (This lock was most recently acquired by a process (pid=65251, host=orbital-3.local, sapi=cli, argv=/Users/epriestley/dev/core/lib/phabricator/bin/ssh-exec --phabricator-ssh-device local.phacility.net --phabricator-ssh-key 2) 2 second(s) ago. There is no record of this lock being released.) Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19702 --- .../DiffusionRepositoryClusterEngine.php | 14 +- .../util/PhabricatorGlobalLock.php | 160 +++++++++++++++++- 2 files changed, 160 insertions(+), 14 deletions(-) diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index d5ba74a30e..b09b3960a1 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -170,12 +170,13 @@ final class DiffusionRepositoryClusterEngine extends Phobject { pht( 'Acquired read lock immediately.')); } - } catch (Exception $ex) { + } catch (PhutilLockException $ex) { throw new PhutilProxyException( pht( 'Failed to acquire read lock after waiting %s second(s). You '. - 'may be able to retry later.', - new PhutilNumber($lock_wait)), + 'may be able to retry later. (%s)', + new PhutilNumber($lock_wait), + $ex->getHint()), $ex); } @@ -349,12 +350,13 @@ final class DiffusionRepositoryClusterEngine extends Phobject { pht( 'Acquired write lock immediately.')); } - } catch (Exception $ex) { + } catch (PhutilLockException $ex) { throw new PhutilProxyException( pht( 'Failed to acquire write lock after waiting %s second(s). You '. - 'may be able to retry later.', - new PhutilNumber($lock_wait)), + 'may be able to retry later. (%s)', + new PhutilNumber($lock_wait), + $ex->getHint()), $ex); } diff --git a/src/infrastructure/util/PhabricatorGlobalLock.php b/src/infrastructure/util/PhabricatorGlobalLock.php index 827d7e0e3d..221a75b37c 100644 --- a/src/infrastructure/util/PhabricatorGlobalLock.php +++ b/src/infrastructure/util/PhabricatorGlobalLock.php @@ -144,7 +144,8 @@ final class PhabricatorGlobalLock extends PhutilLock { $ok = head($result); if (!$ok) { - throw new PhutilLockException($lock_name); + throw id(new PhutilLockException($lock_name)) + ->setHint($this->newHint($lock_name, $wait)); } $conn->rememberLock($lock_name); @@ -152,13 +153,7 @@ final class PhabricatorGlobalLock extends PhutilLock { $this->conn = $conn; if ($this->shouldLogLock()) { - global $argv; - - $lock_context = array( - 'pid' => getmypid(), - 'host' => php_uname('n'), - 'argv' => $argv, - ); + $lock_context = $this->newLockContext(); $log = id(new PhabricatorDaemonLockLog()) ->setLockName($lock_name) @@ -228,4 +223,153 @@ final class PhabricatorGlobalLock extends PhutilLock { return true; } + private function newLockContext() { + $context = array( + 'pid' => getmypid(), + 'host' => php_uname('n'), + 'sapi' => php_sapi_name(), + ); + + global $argv; + if ($argv) { + $context['argv'] = $argv; + } + + $access_log = null; + + // TODO: There's currently no cohesive way to get the parameterized access + // log for the current request across different request types. Web requests + // have an "AccessLog", SSH requests have an "SSHLog", and other processes + // (like scripts) have no log. But there's no method to say "give me any + // log you've got". For now, just test if we have a web request and use the + // "AccessLog" if we do, since that's the only one we actually read any + // parameters from. + + // NOTE: "PhabricatorStartup" is only available from web requests, not + // from CLI scripts. + if (class_exists('PhabricatorStartup', false)) { + $access_log = PhabricatorAccessLog::getLog(); + } + + if ($access_log) { + $controller = $access_log->getData('C'); + if ($controller) { + $context['controller'] = $controller; + } + + $method = $access_log->getData('m'); + if ($method) { + $context['method'] = $method; + } + } + + return $context; + } + + private function newHint($lock_name, $wait) { + if (!$this->shouldLogLock()) { + return pht( + 'Enable the lock log for more detailed information about '. + 'which process is holding this lock.'); + } + + $now = PhabricatorTime::getNow(); + + // First, look for recent logs. If other processes have been acquiring and + // releasing this lock while we've been waiting, this is more likely to be + // a contention/throughput issue than an issue with something hung while + // holding the lock. + $limit = 100; + $logs = id(new PhabricatorDaemonLockLog())->loadAllWhere( + 'lockName = %s AND dateCreated >= %d ORDER BY id ASC LIMIT %d', + $lock_name, + ($now - $wait), + $limit); + + if ($logs) { + if (count($logs) === $limit) { + return pht( + 'During the last %s second(s) spent waiting for the lock, more '. + 'than %s other process(es) acquired it, so this is likely a '. + 'bottleneck. Use "bin/lock log --name %s" to review log activity.', + new PhutilNumber($wait), + new PhutilNumber($limit), + $lock_name); + } else { + return pht( + 'During the last %s second(s) spent waiting for the lock, %s '. + 'other process(es) acquired it, so this is likely a '. + 'bottleneck. Use "bin/lock log --name %s" to review log activity.', + new PhutilNumber($wait), + phutil_count($logs), + $lock_name); + } + } + + $last_log = id(new PhabricatorDaemonLockLog())->loadOneWhere( + 'lockName = %s ORDER BY id DESC LIMIT 1', + $lock_name); + + if ($last_log) { + $info = array(); + + $acquired = $last_log->getDateCreated(); + $context = $last_log->getLockContext(); + + $process_info = array(); + + $pid = idx($context, 'pid'); + if ($pid) { + $process_info[] = 'pid='.$pid; + } + + $host = idx($context, 'host'); + if ($host) { + $process_info[] = 'host='.$host; + } + + $sapi = idx($context, 'sapi'); + if ($sapi) { + $process_info[] = 'sapi='.$sapi; + } + + $argv = idx($context, 'argv'); + if ($argv) { + $process_info[] = 'argv='.(string)csprintf('%LR', $argv); + } + + $controller = idx($context, 'controller'); + if ($controller) { + $process_info[] = 'controller='.$controller; + } + + $method = idx($context, 'method'); + if ($method) { + $process_info[] = 'method='.$method; + } + + $process_info = implode(', ', $process_info); + + $info[] = pht( + 'This lock was most recently acquired by a process (%s) '. + '%s second(s) ago.', + $process_info, + new PhutilNumber($now - $acquired)); + + $released = $last_log->getLockReleased(); + if ($released) { + $info[] = pht( + 'This lock was released %s second(s) ago.', + new PhutilNumber($now - $released)); + } else { + $info[] = pht('There is no record of this lock being released.'); + } + + return implode(' ', $info); + } + + return pht( + 'Found no records of processes acquiring or releasing this lock.'); + } + } From 8065433ee8dcfd31ca544c52b3bcc620db9df13f Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Fri, 17 Aug 2018 12:24:53 -0700 Subject: [PATCH 3/7] Migrate DiffusionBlameController to use repo identities Summary: Now on the blame page, identities get `avatar.png` and there are little tooltips that show a few characters of the committer identity string. Also add a default icon for repo identities. Test Plan: Loaded some blame pages for files touched by users with and without repo identities attached. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19587 --- .../diffusion/controller/DiffusionBlameController.php | 5 +---- .../phid/PhabricatorRepositoryIdentityPHIDType.php | 2 ++ 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/applications/diffusion/controller/DiffusionBlameController.php b/src/applications/diffusion/controller/DiffusionBlameController.php index 42cabbc0d9..95cc039217 100644 --- a/src/applications/diffusion/controller/DiffusionBlameController.php +++ b/src/applications/diffusion/controller/DiffusionBlameController.php @@ -118,10 +118,6 @@ final class DiffusionBlameController extends DiffusionController { $author_phid = $commit->getAuthorDisplayPHID(); } - if (!$author_phid && $revision) { - $author_phid = $revision->getAuthorPHID(); - } - if (!$author_phid) { // This means we couldn't identify an author for the commit or the // revision. We just render a blank for alignment. @@ -137,6 +133,7 @@ final class DiffusionBlameController extends DiffusionController { $author_meta = array( 'tip' => $handles[$author_phid]->getName(), 'align' => 'E', + 'size' => 'auto', ); } diff --git a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php index 5d9b06e0a3..873cfbbae6 100644 --- a/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php +++ b/src/applications/repository/phid/PhabricatorRepositoryIdentityPHIDType.php @@ -30,6 +30,7 @@ final class PhabricatorRepositoryIdentityPHIDType array $handles, array $objects) { + $avatar_uri = celerity_get_resource_uri('/rsrc/image/avatar.png'); foreach ($handles as $phid => $handle) { $identity = $objects[$phid]; @@ -40,6 +41,7 @@ final class PhabricatorRepositoryIdentityPHIDType $handle->setName($name); $handle->setURI($identity->getURI()); $handle->setIcon('fa-user'); + $handle->setImageURI($avatar_uri); } } From dbf2302b6c67a4532e1019b41f80e8c07aee824a Mon Sep 17 00:00:00 2001 From: Austin McKinley Date: Fri, 28 Sep 2018 13:40:26 -0700 Subject: [PATCH 4/7] Fix self-cancelling typo Summary: Ref D18268. This typo cancelled itself out, and I can't find any other callers. Test Plan: arc unit Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19718 --- .../diffusion/protocol/DiffusionRepositoryClusterEngine.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php index b09b3960a1..f85c7862fe 100644 --- a/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php +++ b/src/applications/diffusion/protocol/DiffusionRepositoryClusterEngine.php @@ -208,7 +208,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject { $this->synchronizeWorkingCopyFromDevices($fetchable); } else { - $this->synchornizeWorkingCopyFromRemote(); + $this->synchronizeWorkingCopyFromRemote(); } PhabricatorRepositoryWorkingCopyVersion::updateVersion( @@ -609,7 +609,7 @@ final class DiffusionRepositoryClusterEngine extends Phobject { /** * @task internal */ - private function synchornizeWorkingCopyFromRemote() { + private function synchronizeWorkingCopyFromRemote() { $repository = $this->getRepository(); $device = AlmanacKeys::getLiveDevice(); From 39b85c0be03967b86ee5f3b530d8d0a2e5b3f2de Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Oct 2018 10:00:17 -0700 Subject: [PATCH 5/7] Use the changeset parse cache when rendering inline comments in mail Summary: Ref T13202. See PHI903 and PHI894. When a bot leaves 100 inline comments on the same file and the revision has a 30-member recipient list, we currently highlight the file 3000 times when building mail. Instead, engage the parse cache so we highlight it once and reuse the cache 2,999 times. Test Plan: - Added debugging code to stop after mail generation and show cache hits/misses. - Left a bunch of inlines in a file. - Ran the worker with `bin/worker execute --id ...`. - Before change: saw 4 comments x 2 recipients = 8 cache misses - After change: saw 8 cache hits (cache already filled from web UI rendering) - Removed debugging code, ran worker to completion. - Used `bin/mail show-outbound --id ... --dump-html` to inspect resulting mail, saw no functional differences. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19721 --- .../differential/mail/DifferentialInlineCommentMailView.php | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/applications/differential/mail/DifferentialInlineCommentMailView.php b/src/applications/differential/mail/DifferentialInlineCommentMailView.php index 57958202b1..d88effa46c 100644 --- a/src/applications/differential/mail/DifferentialInlineCommentMailView.php +++ b/src/applications/differential/mail/DifferentialInlineCommentMailView.php @@ -345,7 +345,13 @@ final class DifferentialInlineCommentMailView $offset_mode = 'old'; } + // See PHI894. Use the parse cache since we can end up with a large + // rendering cost otherwise when users or bots leave hundreds of inline + // comments on diffs with long recipient lists. + $cache_key = $changeset->getID(); + $parser = id(new DifferentialChangesetParser()) + ->setRenderCacheKey($cache_key) ->setUser($viewer) ->setChangeset($changeset) ->setOffsetMode($offset_mode) From 4858d43d1601fd93f3af0e9c32fa7930cb1cee58 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Oct 2018 10:49:44 -0700 Subject: [PATCH 6/7] Add 'autocomplete="off"' to MFA TOTP inputs Summary: Ref T13202. See . If browsers are autofilling this, I think browser behavior here is bad, but behavior is probably better on the balance if we hint this as `autocomplete="off"` and this is a minor concesssion. Test Plan: - I couldn't immediately get any browser to try to autofill this field (perhaps I've disabled autofill, or just not enabled it aggressively?), but this change didn't break anything. - After the change, answered a TOTP prompt normally. - After the change, inspected page content and saw `autocomplete="off"` on the `` node. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19722 --- .../auth/factor/PhabricatorTOTPAuthFactor.php | 1 + .../form/control/PHUIFormNumberControl.php | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php index 10c44aaec0..373adfbb54 100644 --- a/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php +++ b/src/applications/auth/factor/PhabricatorTOTPAuthFactor.php @@ -154,6 +154,7 @@ final class PhabricatorTOTPAuthFactor extends PhabricatorAuthFactor { id(new PHUIFormNumberControl()) ->setName($this->getParameterName($config, 'totpcode')) ->setLabel(pht('App Code')) + ->setDisableAutocomplete(true) ->setCaption(pht('Factor Name: %s', $config->getFactorName())) ->setValue(idx($validation_result, 'value')) ->setError(idx($validation_result, 'error', true))); diff --git a/src/view/form/control/PHUIFormNumberControl.php b/src/view/form/control/PHUIFormNumberControl.php index d65e590746..26e7e03955 100644 --- a/src/view/form/control/PHUIFormNumberControl.php +++ b/src/view/form/control/PHUIFormNumberControl.php @@ -2,11 +2,28 @@ final class PHUIFormNumberControl extends AphrontFormControl { + private $disableAutocomplete; + + public function setDisableAutocomplete($disable_autocomplete) { + $this->disableAutocomplete = $disable_autocomplete; + return $this; + } + + public function getDisableAutocomplete() { + return $this->disableAutocomplete; + } + protected function getCustomControlClass() { return 'phui-form-number'; } protected function renderInput() { + if ($this->getDisableAutocomplete()) { + $autocomplete = 'off'; + } else { + $autocomplete = null; + } + return javelin_tag( 'input', array( @@ -15,6 +32,7 @@ final class PHUIFormNumberControl extends AphrontFormControl { 'name' => $this->getName(), 'value' => $this->getValue(), 'disabled' => $this->getDisabled() ? 'disabled' : null, + 'autocomplete' => $autocomplete, 'id' => $this->getID(), )); } From cfd9fa7f559f993e31fcd6915b4857e0de877506 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 1 Oct 2018 11:06:00 -0700 Subject: [PATCH 7/7] Add an explicit "max-width" to PHUIDocumentPro pages to force large tables to scroll Summary: Ref T13202. See . If you put a very wide table in the markup for a new-layout Phriction page, it can push the actions element off screen to the right. Tables already get a scrollbar if encouraged strongly enough; add a `max-width` to encourage them. Test Plan: - Viewed pages with a large wrappable and non-wrappable content on mobile, tablet, and desktop. {F5915976} Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19723 --- resources/celerity/map.php | 4 ++-- webroot/rsrc/css/phui/phui-document-pro.css | 6 ++++++ 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 720683c371..b6817f97eb 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -146,7 +146,7 @@ return array( 'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad', 'rsrc/css/phui/phui-crumbs-view.css' => '10728aaa', 'rsrc/css/phui/phui-curtain-view.css' => '2bdaf026', - 'rsrc/css/phui/phui-document-pro.css' => 'd033e8d5', + 'rsrc/css/phui/phui-document-pro.css' => 'dd79b5df', 'rsrc/css/phui/phui-document-summary.css' => '9ca48bdf', 'rsrc/css/phui/phui-document.css' => 'c4ac41f9', 'rsrc/css/phui/phui-feed-story.css' => '44a9c8e9', @@ -814,7 +814,7 @@ return array( 'phui-curtain-view-css' => '2bdaf026', 'phui-document-summary-view-css' => '9ca48bdf', 'phui-document-view-css' => 'c4ac41f9', - 'phui-document-view-pro-css' => 'd033e8d5', + 'phui-document-view-pro-css' => 'dd79b5df', 'phui-feed-story-css' => '44a9c8e9', 'phui-font-icon-base-css' => '870a7360', 'phui-fontkit-css' => '1320ed01', diff --git a/webroot/rsrc/css/phui/phui-document-pro.css b/webroot/rsrc/css/phui/phui-document-pro.css index 520ae597cb..84dc11601f 100644 --- a/webroot/rsrc/css/phui/phui-document-pro.css +++ b/webroot/rsrc/css/phui/phui-document-pro.css @@ -34,6 +34,12 @@ layout: fixed; } +/* Force very wide content, like tables with many columns, to scroll inside + the frame. See T13202. */ +.phui-document-content-view { + max-width: 800px; +} + .device-desktop .phui-document-content-inner { display: table-row; }