From 98899c822eb98184f25197497cf4ead8dfc9b719 Mon Sep 17 00:00:00 2001 From: Bob Trahan Date: Mon, 16 Mar 2015 16:35:05 -0700 Subject: [PATCH] Conpherence - kill race conditions around update Summary: Fixes T6713. The idea is to keep checking what's going on in the update paths that touch the DOM. If we're doing an update or should be doing a different update, then we bail early. This is the type of code + testing that makes me dizzy after awhile, but I think it works... Test Plan: added a "forceStall" parameter to the column view controller, which when specified sleeps for seconds before returning. I then augmented the JS such that the "send message" code for the durable column would specifiy this parameter. For actual testing, I then spammed the heck out of the durable column channel and saw each message only once. I also spammed the column, switched browsers to a user on the same thread in the normal "speedy" view, sent messages there, and also only received one copy Reviewers: chad, epriestley Reviewed By: epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T6713 Differential Revision: https://secure.phabricator.com/D12092 --- resources/celerity/map.php | 24 +++++----- .../controller/ConpherenceViewController.php | 3 ++ .../conpherence/ConpherenceThreadManager.js | 44 ++++++++++++++++--- 3 files changed, 52 insertions(+), 19 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f2adfb99a8..0e89d55beb 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -352,7 +352,7 @@ return array( 'rsrc/js/application/aphlict/behavior-aphlict-status.js' => 'ea681761', 'rsrc/js/application/auth/behavior-persona-login.js' => '9414ff18', 'rsrc/js/application/config/behavior-reorder-fields.js' => '14a827de', - 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => 'cff1902b', + 'rsrc/js/application/conpherence/ConpherenceThreadManager.js' => 'bbc850a4', 'rsrc/js/application/conpherence/behavior-durable-column.js' => 'e975bd12', 'rsrc/js/application/conpherence/behavior-menu.js' => 'c4151295', 'rsrc/js/application/conpherence/behavior-pontificate.js' => '21ba5861', @@ -517,7 +517,7 @@ return array( 'conpherence-menu-css' => 'c6ac5299', 'conpherence-message-pane-css' => '2526107d', 'conpherence-notification-css' => '04a6e10a', - 'conpherence-thread-manager' => 'cff1902b', + 'conpherence-thread-manager' => 'bbc850a4', 'conpherence-update-css' => '1099a660', 'conpherence-widget-pane-css' => '3d575438', 'differential-changeset-view-css' => '6a8b172a', @@ -1684,6 +1684,16 @@ return array( 'javelin-stratcom', 'javelin-dom', ), + 'bbc850a4' => array( + 'javelin-dom', + 'javelin-util', + 'javelin-stratcom', + 'javelin-install', + 'javelin-workflow', + 'javelin-router', + 'javelin-behavior-device', + 'javelin-vector', + ), 'bbdf75ca' => array( 'javelin-behavior', 'javelin-dom', @@ -1764,16 +1774,6 @@ return array( 'javelin-stratcom', 'phabricator-phtize', ), - 'cff1902b' => array( - 'javelin-dom', - 'javelin-util', - 'javelin-stratcom', - 'javelin-install', - 'javelin-workflow', - 'javelin-router', - 'javelin-behavior-device', - 'javelin-vector', - ), 'd19198c8' => array( 'javelin-install', 'javelin-dom', diff --git a/src/applications/conpherence/controller/ConpherenceViewController.php b/src/applications/conpherence/controller/ConpherenceViewController.php index 7466526323..3283723fa0 100644 --- a/src/applications/conpherence/controller/ConpherenceViewController.php +++ b/src/applications/conpherence/controller/ConpherenceViewController.php @@ -58,6 +58,9 @@ final class ConpherenceViewController extends $content['title'] = $title; if ($request->isAjax()) { + $content['threadID'] = $conpherence->getID(); + $content['threadPHID'] = $conpherence->getPHID(); + $content['latestTransactionID'] = $data['latest_transaction_id']; return id(new AphrontAjaxResponse())->setContent($content); } diff --git a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js index 30b5dab5b3..c83ce96cee 100644 --- a/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js +++ b/webroot/rsrc/js/application/conpherence/ConpherenceThreadManager.js @@ -167,6 +167,23 @@ JX.install('ConpherenceThreadManager', { })); }, + _shouldUpdateDOM: function(r) { + if (this._updating && + this._updating.threadPHID == this._loadedThreadPHID) { + // we have a different, more current update in progress so + // return early + if (r.latest_transaction_id < this._updating.knownID) { + return false; + } + // we need to let the update code handle things here + if (r.latest_transaction_id > this._updating.knownID) { + this._updating.knownID = r.latest_transaction_id; + return false; + } + } + return true; + }, + _updateThread: function() { var params = this._getParams({ action: 'load', @@ -177,7 +194,16 @@ JX.install('ConpherenceThreadManager', { var workflow = new JX.Workflow(uri) .setData(params) .setHandler(JX.bind(this, function(r) { + if (this._updating && + this._updating.threadPHID == this._loadedThreadPHID) { + // we have a different, more current update in progress so + // return early + if (r.latest_transaction_id < this._updating.knownID) { + return; + } + } this._latestTransactionID = r.latest_transaction_id; + this._updating.knownID = r.latest_transaction_id; this._didUpdateThreadCallback(r); })); @@ -191,10 +217,10 @@ JX.install('ConpherenceThreadManager', { }; workflow.listen(stage, JX.bind(this, function() { // TODO - do we need to handle if we switch threads somehow? - var need_sync = (this._updating.knownID > this._latestTransactionID); - this._updating = null; + var need_sync = this._updating && + (this._updating.knownID > this._latestTransactionID); if (need_sync) { - this._updateThread(); + return this._updateThread(); } })); workflow.start(); @@ -206,8 +232,10 @@ JX.install('ConpherenceThreadManager', { var workflow = new JX.Workflow.newFromLink(link) .setData(params) .setHandler(JX.bind(this, function(r) { - this._latestTransactionID = r.latest_transaction_id; - this._didUpdateWorkflowCallback(r); + if (this._shouldUpdateDOM(r)) { + this._latestTransactionID = r.latest_transaction_id; + this._didUpdateWorkflowCallback(r); + } })); this.syncWorkflow(workflow, params.stage); }, @@ -249,8 +277,10 @@ JX.install('ConpherenceThreadManager', { var workflow = JX.Workflow.newFromForm(form, params, keep_enabled) .setHandler(JX.bind(this, function(r) { - this._latestTransactionID = r.latest_transaction_id; - this._didSendMessageCallback(r); + if (this._shouldUpdateDOM(r)) { + this._latestTransactionID = r.latest_transaction_id; + this._didSendMessageCallback(r); + } })); this.syncWorkflow(workflow, 'finally');