From ee1e2da8fbf4c15f1c86675d4fd26567f4137f82 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 14 Apr 2011 09:41:43 -0700 Subject: [PATCH] Avoid Timeline race condition Summary: While I should fix the transactional stuff, that patch is going to be tricky and transactions have some performance implications. This is a simple fix which prevents the race. Instead of having the data point at the event ID, have the event point at a data ID. Insert the data first, then insert the event with the right data pointer. This is super simple and prevents the race issue. Test Plan: - Ran the schema upgrade script, verified that the database was correctly upgraded. Was also prompted to stop daemons. - Ran 'repository-launch-master', verified that the discovery daemons were able to discover new commits and insert events for them. Verified the committask daemon was consuming events and converting them into tasks. - Verified new tasks looked correct in the database. - Browsed web interface. Reviewers: jungejason CC: tuomaspelkonen Differential Revision: 133 --- resources/sql/patches/029.cursors.sql | 13 +++++++++++++ scripts/sql/upgrade_schema.php | 11 +++++++++++ .../PhabricatorDaemonTimelineEventController.php | 7 ++++--- .../cursor/iterator/PhabricatorTimelineIterator.php | 2 +- .../storage/event/PhabricatorTimelineEvent.php | 12 +++++++++--- .../eventdata/PhabricatorTimelineEventData.php | 1 - 6 files changed, 38 insertions(+), 8 deletions(-) create mode 100644 resources/sql/patches/029.cursors.sql diff --git a/resources/sql/patches/029.cursors.sql b/resources/sql/patches/029.cursors.sql new file mode 100644 index 0000000000..0512e45e12 --- /dev/null +++ b/resources/sql/patches/029.cursors.sql @@ -0,0 +1,13 @@ +ALTER TABLE phabricator_timeline.timeline_event + ADD dataID int unsigned; + +ALTER TABLE phabricator_timeline.timeline_event + ADD UNIQUE KEY (dataID); + +UPDATE phabricator_timeline.timeline_event e, + phabricator_timeline.timeline_eventdata d + SET e.dataID = d.id + WHERE d.eventID = e.id; + +ALTER TABLE phabricator_timeline.timeline_eventdata + DROP eventID; diff --git a/scripts/sql/upgrade_schema.php b/scripts/sql/upgrade_schema.php index 3cc2854009..26417647c2 100755 --- a/scripts/sql/upgrade_schema.php +++ b/scripts/sql/upgrade_schema.php @@ -21,6 +21,8 @@ $root = dirname(dirname(dirname(__FILE__))); require_once $root.'/scripts/__init_script__.php'; require_once $root.'/scripts/__init_env__.php'; +phutil_require_module('phutil', 'console'); + const TABLE_NAME = 'schema_version'; if (isset($argv[1]) && !is_numeric($argv[1])) { @@ -33,6 +35,15 @@ if (isset($argv[1]) && !is_numeric($argv[1])) { exit(0); } +echo phutil_console_wrap( + "Before running this script, you should take down the Phabricator web ". + "interface and stop any running Phabricator daemons."); + +if (!phutil_console_confirm('Are you ready to continue?')) { + echo "Cancelled.\n"; + exit(1); +} + // Use always the version from the commandline if it is defined $next_version = isset($argv[1]) ? (int)$argv[1] : null; diff --git a/src/applications/daemon/controller/timelineevent/PhabricatorDaemonTimelineEventController.php b/src/applications/daemon/controller/timelineevent/PhabricatorDaemonTimelineEventController.php index 126d37a1b4..a1499d141d 100644 --- a/src/applications/daemon/controller/timelineevent/PhabricatorDaemonTimelineEventController.php +++ b/src/applications/daemon/controller/timelineevent/PhabricatorDaemonTimelineEventController.php @@ -34,9 +34,10 @@ class PhabricatorDaemonTimelineEventController $request = $this->getRequest(); $user = $request->getUser(); - $data = id(new PhabricatorTimelineEventData())->loadOneWhere( - 'eventID = %d', - $event->getID()); + if ($event->getDataID()) { + $data = id(new PhabricatorTimelineEventData())->load( + $event->getDataID()); + } if ($data) { $data = json_encode($data->getEventData()); diff --git a/src/infrastructure/daemon/timeline/cursor/iterator/PhabricatorTimelineIterator.php b/src/infrastructure/daemon/timeline/cursor/iterator/PhabricatorTimelineIterator.php index 1aadedeefc..0fc1b5bcd4 100644 --- a/src/infrastructure/daemon/timeline/cursor/iterator/PhabricatorTimelineIterator.php +++ b/src/infrastructure/daemon/timeline/cursor/iterator/PhabricatorTimelineIterator.php @@ -54,7 +54,7 @@ class PhabricatorTimelineIterator implements Iterator { $event->establishConnection('r'), 'SELECT event.*, event_data.eventData eventData FROM %T event - LEFT JOIN %T event_data ON event_data.eventID = event.id + LEFT JOIN %T event_data ON event_data.id = event.dataID WHERE event.id > %d AND event.type in (%Ls) ORDER BY event.id ASC LIMIT %d', $event->getTableName(), diff --git a/src/infrastructure/daemon/timeline/storage/event/PhabricatorTimelineEvent.php b/src/infrastructure/daemon/timeline/storage/event/PhabricatorTimelineEvent.php index 966fa4e94d..8ff507624a 100644 --- a/src/infrastructure/daemon/timeline/storage/event/PhabricatorTimelineEvent.php +++ b/src/infrastructure/daemon/timeline/storage/event/PhabricatorTimelineEvent.php @@ -19,6 +19,8 @@ class PhabricatorTimelineEvent extends PhabricatorTimelineDAO { protected $type; + protected $dataID; + private $data; public function __construct($type, $data = null) { @@ -43,14 +45,18 @@ class PhabricatorTimelineEvent extends PhabricatorTimelineDAO { throw new Exception("Event has already been recorded!"); } - $this->save(); - + // Save the data first and point to it from the event to avoid a race + // condition where we insert the event before the data and a consumer reads + // it immediately. if ($this->data !== null) { $data = new PhabricatorTimelineEventData(); - $data->setEventID($this->getID()); $data->setEventData($this->data); $data->save(); + + $this->setDataID($data->getID()); } + + $this->save(); } public function setData($data) { diff --git a/src/infrastructure/daemon/timeline/storage/eventdata/PhabricatorTimelineEventData.php b/src/infrastructure/daemon/timeline/storage/eventdata/PhabricatorTimelineEventData.php index 0bcf9d3035..407d4e1ece 100644 --- a/src/infrastructure/daemon/timeline/storage/eventdata/PhabricatorTimelineEventData.php +++ b/src/infrastructure/daemon/timeline/storage/eventdata/PhabricatorTimelineEventData.php @@ -18,7 +18,6 @@ class PhabricatorTimelineEventData extends PhabricatorTimelineDAO { - protected $eventID; protected $eventData; public function getConfiguration() {