From 5e3efca08a57554535dafd47d55ce38aa83c4b00 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 11 Aug 2016 08:47:47 -0700
Subject: [PATCH] In taskmaster daemons, only close connections which were not
 used recently

Summary:
Ref T11458. Depends on D16388. Currently, we're very aggressive about closing connections in the taskmaster daemons.

This can end up taking up a lot of resources. In particular, because the outgoing port for outbound connections normally can not be reused for 60 seconds after a connection closes, we may exhaust outbound ports on the host if there's a big queue full of stuff that's being processed very quickly.

At a minimum, we //always// are holding open a `worker` connection, which we always need again right away. So even in the best case we end up opening/closing this about once per second and each daemon takes up about ~60 outbound ports when it should take up ~1.

So, make two adjustments:

  - First, only close connections which we haven't issued a query on in the last 60 seconds. This should prevent us from closing connections that we'll need again immediately in most cases. In the worst case, we shouldn't be eating up any extra ports under default TCP behavior.
  - Second, explicitly close connections. We were relying on implicit/GC behavior (maybe as a holdover from very long ago, before we got connection wrappers in place?), which probably did about the same thing but isn't as predictable and can't be profiled or instrumented.

Test Plan:
This is somewhat difficult to test completely convincingly in isolation since the problem behavior depends on production scales and the workload, and to some degree on configuration.

I tested that this stuff baiscally works by adding logging to connect/close and running the daemons, verifying that they churned connections a lot before this change (e.g., ~1/s even at no load) and churn rarely afterward (e.g., almost never at no load).

I ran some workload through them to make sure I didn't completely break anything.

The best real test is just seeing how production responds. Current inbound/outbound connections on `secure001` are 1,200:

```
secure001 $ netstat -t | grep :mysql | wc -l
1164
```

Current outbound from `repo001` are 18,600:

```
repo001 $ netstat -t | grep :mysql | wc -l
18663
```

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T11458

Differential Revision: https://secure.phabricator.com/D16389
---
 .../daemon/PhabricatorDaemon.php              |  2 +-
 src/infrastructure/storage/lisk/LiskDAO.php   | 49 ++++++++++++++++++-
 2 files changed, 48 insertions(+), 3 deletions(-)

diff --git a/src/infrastructure/daemon/PhabricatorDaemon.php b/src/infrastructure/daemon/PhabricatorDaemon.php
index 349e0f0ffe..6fd2ed6016 100644
--- a/src/infrastructure/daemon/PhabricatorDaemon.php
+++ b/src/infrastructure/daemon/PhabricatorDaemon.php
@@ -11,7 +11,7 @@ abstract class PhabricatorDaemon extends PhutilDaemon {
   }
 
   protected function willSleep($duration) {
-    LiskDAO::closeAllConnections();
+    LiskDAO::closeInactiveConnections(60);
     return;
   }
 
diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php
index cf30dc35bc..e795777394 100644
--- a/src/infrastructure/storage/lisk/LiskDAO.php
+++ b/src/infrastructure/storage/lisk/LiskDAO.php
@@ -1621,10 +1621,55 @@ abstract class LiskDAO extends Phobject {
     return (bool)self::$transactionIsolationLevel;
   }
 
-  public static function closeAllConnections() {
-    self::$connections = array();
+  /**
+   * Close any connections with no recent activity.
+   *
+   * Long-running processes can use this method to clean up connections which
+   * have not been used recently.
+   *
+   * @param int Close connections with no activity for this many seconds.
+   * @return void
+   */
+  public static function closeInactiveConnections($idle_window) {
+    $connections = self::$connections;
+
+    $now = PhabricatorTime::getNow();
+    foreach ($connections as $key => $connection) {
+      $last_active = $connection->getLastActiveEpoch();
+
+      $idle_duration = ($now - $last_active);
+      if ($idle_duration <= $idle_window) {
+        continue;
+      }
+
+      self::closeConnection($key);
+    }
   }
 
+
+  public static function closeAllConnections() {
+    $connections = self::$connections;
+
+    foreach ($connections as $key => $connection) {
+      self::closeConnection($key);
+    }
+  }
+
+  private static function closeConnection($key) {
+    if (empty(self::$connections[$key])) {
+      throw new Exception(
+        pht(
+          'No database connection with connection key "%s" exists!',
+          $key));
+    }
+
+    $connection = self::$connections[$key];
+    unset(self::$connections[$key]);
+
+    $connection->close();
+  }
+
+
 /* -(  Utilities  )---------------------------------------------------------- */