From 1089a48d4a66f01401049816ed88e3be034f379b Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 9 Jul 2012 10:39:38 -0700 Subject: [PATCH] Allow edges to be configured to prevent cycles Summary: Certain types of things we should be storing in edges (notably, Task X depends on Task Y) should always be acyclic. Allow `PhabricatorEdgeEditor` to enforce this, since we can't correctly enforce it outside of the editor without being vulnerable to races. Each edge type can be marked acyclic. If an edge type is acyclic, we perform additional steps when writing new edges of that type: - We acquire a global lock on the edge type before performing any reads or writes. This ensures we can't produce a cycle as a result of a race where two edits add edges which independently do not produce a cycle, but do produce a cycle when combined. - After performing writes but before committing transactions, we load the edge graph for each acyclic type and verify that it is, in fact, acyclic. If we detect cycles, we abort the edit. - When we're done, we release the edge type locks. This is a relatively high-complexity change, but gives us a simple way to flag an edge type as acyclic in a robust way. Test Plan: Ran unit tests. Reviewers: btrahan Reviewed By: btrahan CC: aran Maniphest Tasks: T1162 Differential Revision: https://secure.phabricator.com/D2940 --- src/__phutil_library_map__.php | 6 + .../__tests__/PhabricatorEdgeTestCase.php | 81 ++++++++++++ .../edges/constants/PhabricatorEdgeConfig.php | 10 ++ .../edges/editor/PhabricatorEdgeEditor.php | 121 ++++++++++++++++-- .../PhabricatorEdgeCycleException.php | 42 ++++++ .../edges/util/PhabricatorEdgeGraph.php | 50 ++++++++ 6 files changed, 298 insertions(+), 12 deletions(-) create mode 100644 src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php create mode 100644 src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php create mode 100644 src/infrastructure/edges/util/PhabricatorEdgeGraph.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index b2a7ae4559..60ca2aa07f 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -621,8 +621,11 @@ phutil_register_library_map(array( 'PhabricatorDraftDAO' => 'applications/draft/storage/PhabricatorDraftDAO.php', 'PhabricatorEdgeConfig' => 'infrastructure/edges/constants/PhabricatorEdgeConfig.php', 'PhabricatorEdgeConstants' => 'infrastructure/edges/constants/PhabricatorEdgeConstants.php', + 'PhabricatorEdgeCycleException' => 'infrastructure/edges/exception/PhabricatorEdgeCycleException.php', 'PhabricatorEdgeEditor' => 'infrastructure/edges/editor/PhabricatorEdgeEditor.php', + 'PhabricatorEdgeGraph' => 'infrastructure/edges/util/PhabricatorEdgeGraph.php', 'PhabricatorEdgeQuery' => 'infrastructure/edges/query/PhabricatorEdgeQuery.php', + 'PhabricatorEdgeTestCase' => 'infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php', 'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php', 'PhabricatorEmailTokenController' => 'applications/auth/controller/PhabricatorEmailTokenController.php', 'PhabricatorEmailVerificationController' => 'applications/people/controller/PhabricatorEmailVerificationController.php', @@ -1643,7 +1646,10 @@ phutil_register_library_map(array( 'PhabricatorDraft' => 'PhabricatorDraftDAO', 'PhabricatorDraftDAO' => 'PhabricatorLiskDAO', 'PhabricatorEdgeConfig' => 'PhabricatorEdgeConstants', + 'PhabricatorEdgeCycleException' => 'Exception', + 'PhabricatorEdgeGraph' => 'AbstractDirectedGraph', 'PhabricatorEdgeQuery' => 'PhabricatorQuery', + 'PhabricatorEdgeTestCase' => 'PhabricatorTestCase', 'PhabricatorEmailLoginController' => 'PhabricatorAuthController', 'PhabricatorEmailTokenController' => 'PhabricatorAuthController', 'PhabricatorEmailVerificationController' => 'PhabricatorPeopleController', diff --git a/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php new file mode 100644 index 0000000000..ac07e3aaeb --- /dev/null +++ b/src/infrastructure/edges/__tests__/PhabricatorEdgeTestCase.php @@ -0,0 +1,81 @@ + true, + ); + } + + public function testCycleDetection() { + + // The editor should detect that this introduces a cycle and prevent the + // edit. + + $user = new PhabricatorUser(); + + $obj1 = id(new HarbormasterObject())->save(); + $obj2 = id(new HarbormasterObject())->save(); + $phid1 = $obj1->getPHID(); + $phid2 = $obj2->getPHID(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + + + // The first edit should go through (no cycle), bu the second one should + // fail (it introduces a cycle). + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid1, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid2) + ->save(); + + $editor = id(new PhabricatorEdgeEditor()) + ->setUser($user) + ->addEdge($phid2, PhabricatorEdgeConfig::TYPE_TEST_NO_CYCLE, $phid1); + + $caught = null; + try { + $editor->save(); + } catch (Exception $ex) { + $caught = $ex; + } + + $this->assertEqual( + true, + $caught instanceof Exception); + } + + +} diff --git a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php index 0dfa2e34f9..627aa2277a 100644 --- a/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php +++ b/src/infrastructure/edges/constants/PhabricatorEdgeConfig.php @@ -24,6 +24,8 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { const TYPE_TASK_HAS_COMMIT = 1; const TYPE_COMMIT_HAS_TASK = 2; + const TYPE_TEST_NO_CYCLE = 9000; + public static function getInverse($edge_type) { static $map = array( self::TYPE_TASK_HAS_COMMIT => self::TYPE_COMMIT_HAS_TASK, @@ -33,6 +35,13 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { return idx($map, $edge_type); } + public static function shouldPreventCycles($edge_type) { + static $map = array( + self::TYPE_TEST_NO_CYCLE => true, + ); + return isset($map[$edge_type]); + } + public static function establishConnection($phid_type, $conn_type) { static $class_map = array( PhabricatorPHIDConstants::PHID_TYPE_TASK => 'ManiphestTask', @@ -43,6 +52,7 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants { PhabricatorPHIDConstants::PHID_TYPE_PROJ => 'PhabricatorProject', PhabricatorPHIDConstants::PHID_TYPE_MLST => 'PhabricatorMetaMTAMailingList', + PhabricatorPHIDConstants::PHID_TYPE_TOBJ => 'HarbormasterObject', ); $class = idx($class_map, $phid_type); diff --git a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php index 77653e89be..f4f7c811c3 100644 --- a/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php +++ b/src/infrastructure/edges/editor/PhabricatorEdgeEditor.php @@ -32,6 +32,7 @@ * ->save(); * * @task edit Editing Edges + * @task cycles Cycle Prevention * @task internal Internals */ final class PhabricatorEdgeEditor { @@ -113,26 +114,61 @@ final class PhabricatorEdgeEditor { */ public function save() { - // NOTE: We write edge data first, before doing any transactions, since - // it's OK if we just leave it hanging out in space unattached to anything. + $cycle_types = $this->getPreventCyclesEdgeTypes(); - $this->writeEdgeData(); + $locks = array(); + $caught = null; + try { - static $id = 0; - $id++; + // NOTE: We write edge data first, before doing any transactions, since + // it's OK if we just leave it hanging out in space unattached to + // anything. + $this->writeEdgeData(); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); + // If we're going to perform cycle detection, lock the edge type before + // doing edits. + if ($cycle_types) { + $src_phids = ipull($this->addEdges, 'src'); + foreach ($cycle_types as $cycle_type) { + $key = 'edge.cycle:'.$cycle_type; + $locks[] = PhabricatorGlobalLock::newLock($key)->lock(15); + } + } - // NOTE: Removes first, then adds, so that "remove + add" is a useful - // operation meaning "overwrite". + static $id = 0; + $id++; - $this->executeRemoves(); - $this->executeAdds(); + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_WILLEDITEDGES); - $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + // NOTE: Removes first, then adds, so that "remove + add" is a useful + // operation meaning "overwrite". + + $this->executeRemoves(); + $this->executeAdds(); + + foreach ($cycle_types as $cycle_type) { + $this->detectCycles($src_phids, $cycle_type); + } + + $this->sendEvent($id, PhabricatorEventType::TYPE_EDGE_DIDEDITEDGES); + + $this->saveTransactions(); + } catch (Exception $ex) { + $caught = $ex; + } - $this->saveTransactions(); + if ($caught) { + $this->killTransactions(); + } + + foreach ($locks as $lock) { + $lock->unlock(); + } + + if ($caught) { + throw $caught; + } } @@ -327,6 +363,13 @@ final class PhabricatorEdgeEditor { } } + private function killTransactions() { + foreach ($this->openTransactions as $key => $conn_w) { + $conn_w->killTransaction(); + unset($this->openTransactions[$key]); + } + } + private function sendEvent($edit_id, $event_type) { $event = new PhabricatorEvent( $event_type, @@ -339,4 +382,58 @@ final class PhabricatorEdgeEditor { PhutilEventEngine::dispatchEvent($event); } + +/* -( Cycle Prevention )--------------------------------------------------- */ + + + /** + * Get a list of all edge types which are being added, and which we should + * prevent cycles on. + * + * @return list List of edge types which should have cycles prevented. + * @task cycle + */ + private function getPreventCyclesEdgeTypes() { + $edge_types = array(); + foreach ($this->addEdges as $edge) { + $edge_types[$edge['type']] = true; + } + foreach ($edge_types as $type => $ignored) { + if (!PhabricatorEdgeConfig::shouldPreventCycles($type)) { + unset($edge_types[$type]); + } + } + return array_keys($edge_types); + } + + + /** + * Detect graph cycles of a given edge type. If the edit introduces a cycle, + * a @{class:PhabricatorEdgeCycleException} is thrown with details. + * + * @return void + * @task cycle + */ + private function detectCycles(array $phids, $edge_type) { + // For simplicity, we just seed the graph with the affected nodes rather + // than seeding it with their edges. To do this, we just add synthetic + // edges from an imaginary '' node to the known edges. + + + $graph = id(new PhabricatorEdgeGraph()) + ->setEdgeType($edge_type) + ->addNodes( + array( + '' => $phids, + )) + ->loadGraph(); + + foreach ($phids as $phid) { + $cycle = $graph->detectCycles($phid); + if ($cycle) { + throw new PhabricatorEdgeCycleException($edge_type, $cycle); + } + } + } + } diff --git a/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php new file mode 100644 index 0000000000..98c7561748 --- /dev/null +++ b/src/infrastructure/edges/exception/PhabricatorEdgeCycleException.php @@ -0,0 +1,42 @@ +cycleEdgeType = $cycle_edge_type; + $this->cycle = $cycle; + + $cycle_list = implode(', ', $cycle); + + parent::__construct( + "Graph cycle detected (type={$cycle_edge_type}, cycle={$cycle_list})."); + } + + public function getCycle() { + return $this->cycle; + } + + public function getCycleEdgeType() { + return $this->cycleEdgeType; + } + +} diff --git a/src/infrastructure/edges/util/PhabricatorEdgeGraph.php b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php new file mode 100644 index 0000000000..5cd3bfc5bb --- /dev/null +++ b/src/infrastructure/edges/util/PhabricatorEdgeGraph.php @@ -0,0 +1,50 @@ +edgeType = $edge_type; + return $this; + } + + protected function loadEdges(array $nodes) { + if (!$this->edgeType) { + throw new Exception("Set edge type before loading graph!"); + } + + $edges = id(new PhabricatorEdgeQuery()) + ->withSourcePHIDs($nodes) + ->withEdgeTypes(array($this->edgeType)) + ->execute(); + + $results = array_fill_keys($nodes, array()); + foreach ($edges as $src => $types) { + foreach ($types as $type => $dsts) { + foreach ($dsts as $dst => $edge) { + $results[$src][] = $dst; + } + } + } + + return $results; + } + +}