From 53be237d11a415fd799d4e3380b67df3baeb7503 Mon Sep 17 00:00:00 2001
From: Zach Bloomquist <github@chary.us>
Date: Wed, 11 Mar 2020 17:24:48 -0400
Subject: [PATCH] Fix proxy slowdown with intercepted HTTPS requests (#6705)

* use 2x chrome total time as benchmark

* set TLS minVersion to v1 only if connection already failed with TLS version mismatch

* correct percentile function

* assert at least 1000 requests were made

* setNoDelay on HTTPS-over-HTTPS requests

* allow for tests with HTTPS upstreams to be slightly slower

* try 3x

* add note for add'l multiplier on httpsUpstreamProxy
---
 packages/network/lib/agent.ts                 |  5 +----
 packages/server/lib/request.coffee            | 11 ++++++++++-
 .../performance/proxy_performance_spec.js     | 19 ++++++++++++++-----
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/packages/network/lib/agent.ts b/packages/network/lib/agent.ts
index 722667bb5e38..4d9742baf4c5 100644
--- a/packages/network/lib/agent.ts
+++ b/packages/network/lib/agent.ts
@@ -262,10 +262,6 @@ class HttpsAgent extends https.Agent {
   }
 
   createConnection (options: HttpsRequestOptions, cb: http.SocketCallback) {
-    // allow requests to use older TLS versions
-    // https://github.com/cypress-io/cypress/issues/5446
-    options.minVersion = 'TLSv1'
-
     if (process.env.HTTPS_PROXY) {
       const proxy = getProxyForUrl(options.href)
 
@@ -353,6 +349,7 @@ class HttpsAgent extends https.Agent {
 
       const connectReq = buildConnectReqHead(hostname, port, proxy)
 
+      proxySocket.setNoDelay(true)
       proxySocket.write(connectReq)
     })
   }
diff --git a/packages/server/lib/request.coffee b/packages/server/lib/request.coffee
index 40927bff5302..c82e21121a88 100644
--- a/packages/server/lib/request.coffee
+++ b/packages/server/lib/request.coffee
@@ -15,6 +15,7 @@ SERIALIZABLE_COOKIE_PROPS = ['name', 'value', 'domain', 'expiry', 'path', 'secur
 NETWORK_ERRORS = "ECONNREFUSED ECONNRESET EPIPE EHOSTUNREACH EAI_AGAIN ENOTFOUND".split(" ")
 VERBOSE_REQUEST_OPTS = "followRedirect strictSSL".split(" ")
 HTTP_CLIENT_REQUEST_EVENTS = "abort connect continue information socket timeout upgrade".split(" ")
+TLS_VERSION_ERROR_RE =  /TLSV1_ALERT_PROTOCOL_VERSION|UNSUPPORTED_PROTOCOL/
 
 process.env.NODE_TLS_REJECT_UNAUTHORIZED = "0"
 
@@ -81,7 +82,15 @@ maybeRetryOnNetworkFailure = (err, options = {}) ->
 
   debug("received an error making http request %o", merge(opts, { err }))
 
-  if not isRetriableError(err, retryOnNetworkFailure)
+  isTlsVersionError = TLS_VERSION_ERROR_RE.test(err.message)
+
+  if isTlsVersionError
+    ## because doing every connection via TLSv1 can lead to slowdowns, we set it only on failure
+    ## https://github.com/cypress-io/cypress/pull/6705
+    debug('detected TLS version error, setting min version to TLSv1')
+    opts.minVersion = 'TLSv1'
+
+  if not isTlsVersionError and not isRetriableError(err, retryOnNetworkFailure)
     return onElse()
 
   ## else see if we have more delays left...
diff --git a/packages/server/test/performance/proxy_performance_spec.js b/packages/server/test/performance/proxy_performance_spec.js
index 9cdeae85fe93..321d778b0ef9 100644
--- a/packages/server/test/performance/proxy_performance_spec.js
+++ b/packages/server/test/performance/proxy_performance_spec.js
@@ -109,7 +109,7 @@ const average = (arr) => {
 }
 
 const percentile = (sortedArr, p) => {
-  const i = Math.floor(p / 100 * sortedArr.length - 1)
+  const i = Math.floor(p / 100 * (sortedArr.length - 1))
 
   return Math.round(sortedArr[i])
 }
@@ -167,6 +167,8 @@ const getResultsFromHar = (har) => {
 
   results['Min'] = mins.total
 
+  expect(timings.total.length).to.be.at.least(1000)
+
   ;[1, 5, 25, 50, 75, 95, 99, 99.7].forEach((p) => {
     results[`${p}% <=`] = percentile(timings.total, p)
   })
@@ -356,10 +358,9 @@ describe('Proxy Performance', function () {
   })
 
   URLS_UNDER_TEST.map((urlUnderTest) => {
-    const testCases = _.cloneDeep(TEST_CASES)
-
     describe(urlUnderTest, function () {
       let baseline
+      const testCases = _.cloneDeep(TEST_CASES)
 
       before(function () {
         // run baseline test
@@ -373,12 +374,20 @@ describe('Proxy Performance', function () {
 
       // slice(1) since first test is used as baseline above
       testCases.slice(1).map((testCase) => {
-        it(`${testCase.name} loads 1000 images, with 75% loading no more than 2x as slow as the slowest baseline request`, function () {
+        let multiplier = 3
+
+        if (testCase.httpsUpstreamProxy) {
+          // there is extra slowdown when the HTTPS upstream is used, so slightly increase the multiplier
+          // maybe from higher CPU utilization with debugging-proxy and HTTPS
+          multiplier *= 1.5
+        }
+
+        it(`${testCase.name} loads 1000 images less than ${multiplier}x as slowly as Chrome`, function () {
           debug('Current test: ', testCase.name)
 
           return runBrowserTest(urlUnderTest, testCase)
           .then((results) => {
-            expect(results['75% <=']).to.be.lessThan(baseline['Max'] * 2)
+            expect(results['Total']).to.be.lessThan(multiplier * baseline['Total'])
           })
         })
       })