Begin improving modularity of IndexEngine, add locks

Summary:
Ref T9890. Ref T9979. Several adjacent goals:

  - The `SearchEngine` vs `ApplicationSearchEngine` thing is really confusing. There are also a bunch of confusing class names and class relationships within the fulltext indexing. I want to rename these classes to be more standard (`IndexEngine`, `IndexEngineExtension`, etc). Rename `SearchIndexer` to `IndexEngine`. A future change will rename `SearchEngine`.
  - Add the index locks described in T9890.
  - Structure things a little more normally so future diffs can do the "EngineExtension" thing more cleanly.

Test Plan:
Indexing:

  - Renamed a task to have a unique word in the title.
  - Ran `bin/search index Txxx`.
  - Searched for unique word.
  - Found task.

Locking:

  - Added a `sleep(10)` after the `lock()` call.
  - Ran `bin/search index Txxx` in two windows.
  - Saw first one lock, sleep 10 seconds, index.
  - Saw second one give up temporarily after failing to grab the lock.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9890, T9979

Differential Revision: https://secure.phabricator.com/D14834

src/__phutil_library_map__.php

@@ -2374,6 +2374,7 @@ phutil_register_library_map(array(
     'PhabricatorImageMacroRemarkupRule' => 'applications/macro/markup/PhabricatorImageMacroRemarkupRule.php',
     'PhabricatorImageTransformer' => 'applications/files/PhabricatorImageTransformer.php',
     'PhabricatorImagemagickSetupCheck' => 'applications/config/check/PhabricatorImagemagickSetupCheck.php',
+    'PhabricatorIndexEngine' => 'applications/search/index/PhabricatorIndexEngine.php',
     'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php',
     'PhabricatorInlineCommentInterface' => 'infrastructure/diff/interface/PhabricatorInlineCommentInterface.php',
@@ -3032,7 +3033,6 @@ phutil_register_library_map(array(
     'PhabricatorSearchEngineTestCase' => 'applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php',
     'PhabricatorSearchField' => 'applications/search/field/PhabricatorSearchField.php',
     'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php',
-    'PhabricatorSearchIndexer' => 'applications/search/index/PhabricatorSearchIndexer.php',
     'PhabricatorSearchManagementIndexWorkflow' => 'applications/search/management/PhabricatorSearchManagementIndexWorkflow.php',
     'PhabricatorSearchManagementInitWorkflow' => 'applications/search/management/PhabricatorSearchManagementInitWorkflow.php',
     'PhabricatorSearchManagementWorkflow' => 'applications/search/management/PhabricatorSearchManagementWorkflow.php',
@@ -6596,6 +6596,7 @@ phutil_register_library_map(array(
     'PhabricatorImageMacroRemarkupRule' => 'PhutilRemarkupRule',
     'PhabricatorImageTransformer' => 'Phobject',
     'PhabricatorImagemagickSetupCheck' => 'PhabricatorSetupCheck',
+    'PhabricatorIndexEngine' => 'Phobject',
     'PhabricatorInfrastructureTestCase' => 'PhabricatorTestCase',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
     'PhabricatorInlineCommentInterface' => 'PhabricatorMarkupInterface',
@@ -7378,7 +7379,6 @@ phutil_register_library_map(array(
     'PhabricatorSearchEngineTestCase' => 'PhabricatorTestCase',
     'PhabricatorSearchField' => 'Phobject',
     'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController',
-    'PhabricatorSearchIndexer' => 'Phobject',
     'PhabricatorSearchManagementIndexWorkflow' => 'PhabricatorSearchManagementWorkflow',
     'PhabricatorSearchManagementInitWorkflow' => 'PhabricatorSearchManagementWorkflow',
     'PhabricatorSearchManagementWorkflow' => 'PhabricatorManagementWorkflow',

src/applications/diviner/publisher/DivinerLivePublisher.php

@@ -40,8 +40,7 @@ final class DivinerLivePublisher extends DivinerPublisher {
       $conn_w->saveTransaction();
       $this->book = $book;
 
-      id(new PhabricatorSearchIndexer())
-        ->queueDocumentForIndexing($book->getPHID());
+      PhabricatorSearchWorker::queueDocumentForIndexing($book->getPHID());
     }
 
     return $this->book;
@@ -147,8 +146,7 @@ final class DivinerLivePublisher extends DivinerPublisher {
 
       $symbol->save();
 
-      id(new PhabricatorSearchIndexer())
-        ->queueDocumentForIndexing($symbol->getPHID());
+      PhabricatorSearchWorker::queueDocumentForIndexing($symbol->getPHID());
 
       // TODO: We probably need a finer-grained sense of what "documentable"
       // atoms are. Neither files nor methods are currently considered

src/applications/people/storage/PhabricatorUser.php

@@ -273,8 +273,7 @@ final class PhabricatorUser
 
     $this->updateNameTokens();
 
-    id(new PhabricatorSearchIndexer())
-      ->queueDocumentForIndexing($this->getPHID());
+    PhabricatorSearchWorker::queueDocumentForIndexing($this->getPHID());
 
     return $result;
   }

src/applications/repository/worker/commitchangeparser/PhabricatorRepositoryCommitChangeParserWorker.php

@@ -93,8 +93,7 @@ abstract class PhabricatorRepositoryCommitChangeParserWorker
     $commit->writeImportStatusFlag(
       PhabricatorRepositoryCommit::IMPORTED_CHANGE);
 
-    id(new PhabricatorSearchIndexer())
-      ->queueDocumentForIndexing($commit->getPHID());
+    PhabricatorSearchWorker::queueDocumentForIndexing($commit->getPHID());
 
     if ($this->shouldQueueFollowupTasks()) {
       $this->queueTask(

src/applications/search/index/PhabricatorIndexEngine.php

@@ -1,18 +1,6 @@
 <?php
 
-final class PhabricatorSearchIndexer extends Phobject {
-
-  public function queueDocumentForIndexing($phid, $context = null) {
-    PhabricatorWorker::scheduleTask(
-      'PhabricatorSearchWorker',
-      array(
-        'documentPHID' => $phid,
-        'context' => $context,
-      ),
-      array(
-        'priority' => PhabricatorWorker::PRIORITY_IMPORT,
-      ));
-  }
+final class PhabricatorIndexEngine extends Phobject {
 
   public function indexDocumentByPHID($phid, $context) {
     $indexers = id(new PhutilClassMapQuery())

src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php

@@ -94,10 +94,9 @@ final class PhabricatorSearchManagementIndexWorkflow
       ->setTotal(count($phids));
 
     $any_success = false;
-    $indexer = new PhabricatorSearchIndexer();
     foreach ($phids as $phid) {
       try {
-        $indexer->queueDocumentForIndexing($phid);
+        PhabricatorSearchWorker::queueDocumentForIndexing($phid);
         $any_success = true;
       } catch (Exception $ex) {
         phlog($ex);

src/applications/search/worker/PhabricatorSearchWorker.php

@@ -2,22 +2,68 @@
 
 final class PhabricatorSearchWorker extends PhabricatorWorker {
 
+  public static function queueDocumentForIndexing($phid, $context = null) {
+    parent::scheduleTask(
+      __CLASS__,
+      array(
+        'documentPHID' => $phid,
+        'context' => $context,
+      ),
+      array(
+        'priority' => parent::PRIORITY_IMPORT,
+      ));
+  }
+
   protected function doWork() {
     $data = $this->getTaskData();
-
-    $phid = idx($data, 'documentPHID');
+    $object_phid = idx($data, 'documentPHID');
     $context = idx($data, 'context');
 
+    $engine = new PhabricatorIndexEngine();
+
+    $key = "index.{$object_phid}";
+    $lock = PhabricatorGlobalLock::newLock($key);
+
+    $lock->lock(1);
+
     try {
-      id(new PhabricatorSearchIndexer())
-        ->indexDocumentByPHID($phid, $context);
+      $object = $this->loadObjectForIndexing($object_phid);
+
+      $engine->indexDocumentByPHID($object->getPHID(), $context);
+
     } catch (Exception $ex) {
+      $lock->unlock();
+
+      if (!($ex instanceof PhabricatorWorkerPermanentFailureException)) {
+        $ex = new PhabricatorWorkerPermanentFailureException(
+          pht(
+            'Failed to update search index for document "%s": %s',
+            $object_phid,
+            $ex->getMessage()));
+      }
+
+      throw $ex;
+    }
+
+    $lock->unlock();
+  }
+
+  private function loadObjectForIndexing($phid) {
+    $viewer = PhabricatorUser::getOmnipotentUser();
+
+    $object = id(new PhabricatorObjectQuery())
+      ->setViewer($viewer)
+      ->withPHIDs(array($phid))
+      ->executeOne();
+
+    if (!$object) {
       throw new PhabricatorWorkerPermanentFailureException(
         pht(
-          'Failed to update search index for document "%s": %s',
-          $phid,
-          $ex->getMessage()));
+          'Unable to load object "%s" to rebuild indexes.',
+          $phid));
     }
+
+    return $object;
   }
 
 }

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1093,10 +1093,9 @@ abstract class PhabricatorApplicationTransactionEditor
     }
 
     if ($this->supportsSearch()) {
-      id(new PhabricatorSearchIndexer())
-        ->queueDocumentForIndexing(
-          $object->getPHID(),
-          $this->getSearchContextParameter($object, $xactions));
+      PhabricatorSearchWorker::queueDocumentForIndexing(
+        $object->getPHID(),
+        $this->getSearchContextParameter($object, $xactions));
     }
 
     if ($this->shouldPublishFeedStory($object, $xactions)) {