From 2f0e655a9b9e9543e02879cfcf007afa5e59b2b0 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Mon, 20 May 2019 11:40:05 -0700
Subject: [PATCH] Add a 15-second timeout to external service calls to fill
 Doorkeeper link tags

Summary:
Depends on D20528. Ref T13291. Ref T13285. Currently, we don't put a timeout on external service calls when enriching URIs for external Asana/JIRA tasks.

Add a 15-second timeout so we'll do something reasonable-ish in the face of a downed service provider. Later, I plan to healthcheck Asana/JIRA providers in a generic way (see T13287) so we can stop making calls if they time out / fail too frequently.

Test Plan:
  - Linked to JIRA and Asana tasks in comments.
  - Set timeout to 0.0001 seconds, saw requests time out.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13291, T13285

Differential Revision: https://secure.phabricator.com/D20530
---
 .../doorkeeper/bridge/DoorkeeperBridge.php    | 10 +++++++++
 .../bridge/DoorkeeperBridgeAsana.php          |  5 +++++
 .../bridge/DoorkeeperBridgeJIRA.php           | 10 ++++++++-
 .../controller/DoorkeeperTagsController.php   |  1 +
 .../engine/DoorkeeperImportEngine.php         | 22 ++++++++++++++++---
 5 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php b/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
index 4a4ee2667b..d25d48e857 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridge.php
@@ -5,6 +5,16 @@ abstract class DoorkeeperBridge extends Phobject {
   private $viewer;
   private $context = array();
   private $throwOnMissingLink;
+  private $timeout;
+
+  public function setTimeout($timeout) {
+    $this->timeout = $timeout;
+    return $this;
+  }
+
+  public function getTimeout() {
+    return $this->timeout;
+  }
 
   public function setThrowOnMissingLink($throw_on_missing_link) {
     $this->throwOnMissingLink = $throw_on_missing_link;
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
index ec604e158e..05ee786337 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeAsana.php
@@ -62,6 +62,11 @@ final class DoorkeeperBridgeAsana extends DoorkeeperBridge {
     $template = id(new PhutilAsanaFuture())
       ->setAccessToken($token);
 
+    $timeout = $this->getTimeout();
+    if ($timeout !== null) {
+      $template->setTimeout($timeout);
+    }
+
     $futures = array();
     foreach ($id_map as $key => $id) {
       $futures[$key] = id(clone $template)
diff --git a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
index 920f2eeb91..f82bb1ba25 100644
--- a/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
+++ b/src/applications/doorkeeper/bridge/DoorkeeperBridgeJIRA.php
@@ -47,12 +47,20 @@ final class DoorkeeperBridgeJIRA extends DoorkeeperBridge {
     // (by querying all instances). For now, just query the one instance.
     $account = head($accounts);
 
+    $timeout = $this->getTimeout();
+
     $futures = array();
     foreach ($id_map as $key => $id) {
-      $futures[$key] = $provider->newJIRAFuture(
+      $future = $provider->newJIRAFuture(
         $account,
         'rest/api/2/issue/'.phutil_escape_uri($id),
         'GET');
+
+      if ($timeout !== null) {
+        $future->setTimeout($timeout);
+      }
+
+      $futures[$key] = $future;
     }
 
     $results = array();
diff --git a/src/applications/doorkeeper/controller/DoorkeeperTagsController.php b/src/applications/doorkeeper/controller/DoorkeeperTagsController.php
index 5a886cba3e..f4b4195f11 100644
--- a/src/applications/doorkeeper/controller/DoorkeeperTagsController.php
+++ b/src/applications/doorkeeper/controller/DoorkeeperTagsController.php
@@ -26,6 +26,7 @@ final class DoorkeeperTagsController extends PhabricatorController {
     $refs = id(new DoorkeeperImportEngine())
       ->setViewer($viewer)
       ->setRefs($refs)
+      ->setTimeout(15)
       ->execute();
 
     $results = array();
diff --git a/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php b/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php
index 8c58f5bd88..02642cb994 100644
--- a/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php
+++ b/src/applications/doorkeeper/engine/DoorkeeperImportEngine.php
@@ -8,6 +8,7 @@ final class DoorkeeperImportEngine extends Phobject {
   private $localOnly;
   private $throwOnMissingLink;
   private $context = array();
+  private $timeout;
 
   public function setViewer(PhabricatorUser $viewer) {
     $this->viewer = $viewer;
@@ -43,6 +44,15 @@ final class DoorkeeperImportEngine extends Phobject {
     return $this;
   }
 
+  public function setTimeout($timeout) {
+    $this->timeout = $timeout;
+    return $this;
+  }
+
+  public function getTimeout() {
+    return $this->timeout;
+  }
+
   /**
    * Configure behavior if remote refs can not be retrieved because an
    * authentication link is missing.
@@ -98,10 +108,16 @@ final class DoorkeeperImportEngine extends Phobject {
         ->setFilterMethod('isEnabled')
         ->execute();
 
+      $timeout = $this->getTimeout();
       foreach ($bridges as $key => $bridge) {
-        $bridge->setViewer($viewer);
-        $bridge->setThrowOnMissingLink($this->throwOnMissingLink);
-        $bridge->setContext($this->context);
+        $bridge
+          ->setViewer($viewer)
+          ->setThrowOnMissingLink($this->throwOnMissingLink)
+          ->setContext($this->context);
+
+        if ($timeout !== null) {
+          $bridge->setTimeout($timeout);
+        }
       }
 
       $working_set = $refs;