From d70ca6b7c8b55b3b2ed294550303d7b051ef90c6 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 21 May 2015 09:42:20 -0700
Subject: [PATCH] When file transforms race and lose, accept defeat gracefully

Summary: Fixes T8277. Transforming files can race; resolve the race after we lose.

Test Plan:
  - Added `sleep(10)` near the bottom of the transform controller.
  - Transformed a file in two browser windows at the same time; got something like this (exception corresponds to the loser of the race):

{F412526}

  - Applied patch.
  - Repeated process, got this:

{F412527}

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T8277

Differential Revision: https://secure.phabricator.com/D12965
---
 .../PhabricatorFileTransformController.php    | 33 ++++++++++++++-----
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/src/applications/files/controller/PhabricatorFileTransformController.php b/src/applications/files/controller/PhabricatorFileTransformController.php
index 0e7e2b73d7..77676c1b55 100644
--- a/src/applications/files/controller/PhabricatorFileTransformController.php
+++ b/src/applications/files/controller/PhabricatorFileTransformController.php
@@ -30,11 +30,7 @@ final class PhabricatorFileTransformController
     }
 
     $transform = $request->getURIData('transform');
-    $xform = id(new PhabricatorTransformedFile())
-      ->loadOneWhere(
-        'originalPHID = %s AND transform = %s',
-        $source_phid,
-        $transform);
+    $xform = $this->loadTransform($source_phid, $transform);
 
     if ($xform) {
       if ($is_regenerate) {
@@ -80,8 +76,20 @@ final class PhabricatorFileTransformController
     $xform = id(new PhabricatorTransformedFile())
       ->setOriginalPHID($source_phid)
       ->setTransform($transform)
-      ->setTransformedPHID($xformed_file->getPHID())
-      ->save();
+      ->setTransformedPHID($xformed_file->getPHID());
+
+    try {
+      $xform->save();
+    } catch (AphrontDuplicateKeyQueryException $ex) {
+      // If we collide when saving, we've raced another endpoint which was
+      // transforming the same file. Just throw our work away and use that
+      // transform instead.
+      $this->destroyTransform($xform);
+      $xform = $this->loadTransform($source_phid, $transform);
+      if (!$xform) {
+        return new Aphront404Response();
+      }
+    }
 
     return $this->buildTransformedFileResponse($xform);
   }
@@ -113,7 +121,9 @@ final class PhabricatorFileTransformController
     $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites();
 
     if (!$file) {
-      $xform->delete();
+      if ($xform->getID()) {
+        $xform->delete();
+      }
     } else {
       $engine->destroyObject($file);
     }
@@ -121,4 +131,11 @@ final class PhabricatorFileTransformController
     unset($unguarded);
   }
 
+  private function loadTransform($source_phid, $transform) {
+    return id(new PhabricatorTransformedFile())->loadOneWhere(
+      'originalPHID = %s AND transform = %s',
+      $source_phid,
+      $transform);
+  }
+
 }