From d84f866ca06127a9e322383f4efdfe4f82ab15cd Mon Sep 17 00:00:00 2001 From: epriestley Date: Fri, 22 Jun 2018 10:01:44 -0700 Subject: [PATCH] When search indexers contend for a lock, just yield Summary: Depends on D19503. Ref T13151. See PHI719. If you have something like a script which updates an object in a loop, we can end up queueing many search reindex tasks. These tasks may reasonably contend for the lock, especially if the object is larger (lots of text and/or lots of comments) and indexing takes a few seconds. This isn't concerning, and the indexers should converge to good behavior quickly once the updates stop. Today, they'll spew a bunch of serious-looking lock exceptions into the log. Instead, just yield so it's more clear that there's (normally) no cause for concern here. Test Plan: Ran `bin/search index Txxx --force` on a large object in multiple windows with a 0 second lock, saw an explicit yield instead of a lock exception. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151 Differential Revision: https://secure.phabricator.com/D19504 --- .../search/worker/PhabricatorSearchWorker.php | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/applications/search/worker/PhabricatorSearchWorker.php b/src/applications/search/worker/PhabricatorSearchWorker.php index 6cebb64da4..f93df63981 100644 --- a/src/applications/search/worker/PhabricatorSearchWorker.php +++ b/src/applications/search/worker/PhabricatorSearchWorker.php @@ -38,7 +38,15 @@ final class PhabricatorSearchWorker extends PhabricatorWorker { $key = "index.{$object_phid}"; $lock = PhabricatorGlobalLock::newLock($key); - $lock->lock(1); + try { + $lock->lock(1); + } catch (PhutilLockException $ex) { + // If we fail to acquire the lock, just yield. It's expected that we may + // contend on this lock occasionally if a large object receives many + // updates in a short period of time, and it's appropriate to just retry + // rebuilding the index later. + throw new PhabricatorWorkerYieldException(15); + } try { // Reload the object now that we have a lock, to make sure we have the