From 138efb2b1035a2cb1ee62ccb54cdae96449347e6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Wed, 7 Sep 2016 10:46:30 -0700
Subject: [PATCH] Recover from a race when importing external objects (like
 JIRA issues) for the first time

Summary:
Fixes T11604. If we send two requests to render a brand new tag at about the same time (say, 50ms apart) but JIRA takes more than 50ms to return from its API call, the two processes will race one another and try to save the same external object.

If they do, have whichever one lost the race just load the object the other one created.

Apply this to other bridges, too.

Test Plan:
  - Created a new task in JIRA.
  - Referenced it for the first time in Differential, in a comment.
  - This causes two tag renders to fire. This //might// be a bug but I spend 30 seconds on it without figuring out what was up. Regardless, we should fix the race even if the reason it's triggering so easily legitimately is a bug.
  - Before patch: big error dialog (as in T11604).
  - After patch: smooth sailing.

{F1804008}

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11604

Differential Revision: https://secure.phabricator.com/D16514
---
 .../doorkeeper/bridge/DoorkeeperBridge.php    | 28 +++++++++++++++++++
 .../bridge/DoorkeeperBridgeAsana.php          |  5 +---
 .../bridge/DoorkeeperBridgeGitHubIssue.php    |  5 +---
 .../bridge/DoorkeeperBridgeGitHubUser.php     |  5 +---
 .../bridge/DoorkeeperBridgeJIRA.php           |  5 +---
 5 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php b/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
index b09a1933c1..4a4ee2667b 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
@@ -48,4 +48,32 @@ abstract class DoorkeeperBridge extends Phobject {
     return null;
   }
 
+  final protected function saveExternalObject(
+    DoorkeeperObjectRef $ref,
+    DoorkeeperExternalObject $obj) {
+
+    $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
+      try {
+        $obj->save();
+      } catch (AphrontDuplicateKeyQueryException $ex) {
+
+        // In various cases, we may race another process importing the same
+        // data. If we do, we'll collide on the object key. Load the object
+        // the other process created and use that.
+        $obj = id(new DoorkeeperExternalObjectQuery())
+          ->setViewer($this->getViewer())
+          ->withObjectKeys(array($ref->getObjectKey()))
+          ->executeOne();
+        if (!$obj) {
+          throw new PhutilProxyException(
+            pht('Failed to load external object after collision.'),
+            $ex);
+        }
+
+        $ref->attachExternalObject($obj);
+      }
+    unset($unguarded);
+  }
+
+
 }
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
index 4e32239793..ec604e158e 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
@@ -113,10 +113,7 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge {
       }
 
       $this->fillObjectFromData($obj, $result);
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $obj->save();
-      unset($unguarded);
+      $this->saveExternalObject($ref, $obj);
     }
   }
 
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php
index 0558b7d8f6..b5d7ddeea5 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubIssue.php
@@ -78,10 +78,7 @@ final class DoorkeeperBridgeGitHubIssue
       $obj = $ref->getExternalObject();
 
       $this->fillObjectFromData($obj, $result);
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $obj->save();
-      unset($unguarded);
+      $this->saveExternalObject($ref, $obj);
     }
   }
 
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php
index 3470894e5c..ef31826eb3 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeGitHubUser.php
@@ -101,10 +101,7 @@ final class DoorkeeperBridgeGitHubUser
 
       $obj = $ref->getExternalObject();
       $this->fillObjectFromData($obj, $spec);
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $obj->save();
-      unset($unguarded);
+      $this->saveExternalObject($ref, $obj);
     }
   }
 
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
index c9b40d2e72..199f049dce 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
@@ -104,10 +104,7 @@ final class DoorkeeperBridgeJIRA extends DoorkeeperBridge {
       }
 
       $this->fillObjectFromData($obj, $result);
-
-      $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
-        $obj->save();
-      unset($unguarded);
+      $this->saveExternalObject($ref, $obj);
     }
   }