fix(proxy/prerequests): fix duplicate key behavior, fallthrough (#23227)

Co-authored-by: Blue F <[email protected]>

Author: flotwig

packages/proxy/lib/http/util/prerequests.ts

@@ -24,7 +24,50 @@ export type GetPreRequestCb = (browserPreRequest?: BrowserPreRequest) => void
 type PendingRequest = {
   ctxDebug
   callback: GetPreRequestCb
-  timeout: ReturnType<typeof setTimeout>
+  timeout: NodeJS.Timeout
+}
+
+type PendingPreRequest = {
+  browserPreRequest: BrowserPreRequest
+  timestamp: number
+}
+
+/**
+ * Data structure that organizes items with duplicated keys into stacks.
+ */
+class StackMap<T> {
+  private stacks: Record<string, Array<T>> = {}
+  push (stackKey: string, value: T) {
+    if (!this.stacks[stackKey]) this.stacks[stackKey] = []
+
+    this.stacks[stackKey].push(value)
+  }
+  pop (stackKey: string): T | undefined {
+    const stack = this.stacks[stackKey]
+
+    if (!stack) return
+
+    const item = stack.pop()
+
+    if (stack.length === 0) delete this.stacks[stackKey]
+
+    return item
+  }
+  removeMatching (filterFn: (value: T) => boolean) {
+    Object.entries(this.stacks).forEach(([stackKey, stack]) => {
+      this.stacks[stackKey] = stack.filter(filterFn)
+      if (this.stacks[stackKey].length === 0) delete this.stacks[stackKey]
+    })
+  }
+  removeExact (stackKey: string, value: T) {
+    const i = this.stacks[stackKey].findIndex((v) => v === value)
+
+    this.stacks[stackKey].splice(i, 1)
+    if (this.stacks[stackKey].length === 0) delete this.stacks[stackKey]
+  }
+  get length () {
+    return Object.values(this.stacks).reduce((prev, cur) => prev + cur.length, 0)
+  }
 }
 
 // This class' purpose is to match up incoming "requests" (requests from the browser received by the http proxy)
@@ -35,9 +78,8 @@ type PendingRequest = {
 // ever comes in, we don't want to block proxied requests indefinitely.
 export class PreRequests {
   requestTimeout: number
-  pendingPreRequests: Record<string, BrowserPreRequest> = {}
-  pendingRequests: Record<string, PendingRequest> = {}
-  prerequestTimestamps: Record<string, number> = {}
+  pendingPreRequests = new StackMap<PendingPreRequest>()
+  pendingRequests = new StackMap<PendingRequest>()
   sweepInterval: ReturnType<typeof setInterval>
 
   constructor (requestTimeout = 500) {
@@ -55,59 +97,63 @@ export class PreRequests {
     this.sweepInterval = setInterval(() => {
       const now = Date.now()
 
-      Object.entries(this.prerequestTimestamps).forEach(([key, timestamp]) => {
+      this.pendingPreRequests.removeMatching(({ timestamp, browserPreRequest }) => {
         if (timestamp + this.requestTimeout * 2 < now) {
-          debugVerbose('timed out unmatched pre-request %s: %o', key, this.pendingPreRequests[key])
+          debugVerbose('timed out unmatched pre-request: %o', browserPreRequest)
           metrics.unmatchedPreRequests++
-          delete this.pendingPreRequests[key]
-          delete this.prerequestTimestamps[key]
+
+          return false
         }
+
+        return true
       })
     }, this.requestTimeout * 2)
   }
 
   addPending (browserPreRequest: BrowserPreRequest) {
     metrics.browserPreRequestsReceived++
     const key = `${browserPreRequest.method}-${browserPreRequest.url}`
+    const pendingRequest = this.pendingRequests.pop(key)
 
-    if (this.pendingRequests[key]) {
+    if (pendingRequest) {
       debugVerbose('Incoming pre-request %s matches pending request. %o', key, browserPreRequest)
-      clearTimeout(this.pendingRequests[key].timeout)
-      this.pendingRequests[key].callback(browserPreRequest)
-      delete this.pendingRequests[key]
+      clearTimeout(pendingRequest.timeout)
+      pendingRequest.callback(browserPreRequest)
+
+      return
     }
 
     debugVerbose('Caching pre-request %s to be matched later. %o', key, browserPreRequest)
-    this.pendingPreRequests[key] = browserPreRequest
-    this.prerequestTimestamps[key] = Date.now()
+    this.pendingPreRequests.push(key, {
+      browserPreRequest,
+      timestamp: Date.now(),
+    })
   }
 
   get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {
     metrics.proxyRequestsReceived++
     const key = `${req.method}-${req.proxiedUrl}`
+    const pendingPreRequest = this.pendingPreRequests.pop(key)
 
-    if (this.pendingPreRequests[key]) {
+    if (pendingPreRequest) {
       metrics.immediatelyMatchedRequests++
-      ctxDebug('Incoming request %s matches known pre-request: %o', key, this.pendingPreRequests[key])
-      callback(this.pendingPreRequests[key])
-
-      delete this.pendingPreRequests[key]
-      delete this.prerequestTimestamps[key]
+      ctxDebug('Incoming request %s matches known pre-request: %o', key, pendingPreRequest)
+      callback(pendingPreRequest.browserPreRequest)
 
       return
     }
 
-    const timeout = setTimeout(() => {
-      callback()
-      ctxDebug('Never received pre-request for request %s after waiting %sms. Continuing without one.', key, this.requestTimeout)
-      metrics.unmatchedRequests++
-      delete this.pendingRequests[key]
-    }, this.requestTimeout)
-
-    this.pendingRequests[key] = {
+    const pendingRequest: PendingRequest = {
       ctxDebug,
       callback,
-      timeout,
+      timeout: setTimeout(() => {
+        ctxDebug('Never received pre-request for request %s after waiting %sms. Continuing without one.', key, this.requestTimeout)
+        metrics.unmatchedRequests++
+        this.pendingRequests.removeExact(key, pendingRequest)
+        callback()
+      }, this.requestTimeout),
     }
+
+    this.pendingRequests.push(key, pendingRequest)
   }
 }

packages/proxy/test/unit/http/util/prerequests.spec.ts

@@ -6,6 +6,11 @@ import sinon from 'sinon'
 describe('http/util/prerequests', () => {
   let preRequests: PreRequests
 
+  function expectPendingCounts (pendingRequests: number, pendingPreRequests: number) {
+    expect(preRequests.pendingRequests.length).to.eq(pendingRequests, 'wrong number of pending requests')
+    expect(preRequests.pendingPreRequests.length).to.eq(pendingPreRequests, 'wrong number of pending prerequests')
+  }
+
   beforeEach(() => {
     preRequests = new PreRequests(10)
   })
@@ -18,19 +23,26 @@ describe('http/util/prerequests', () => {
     // should match in reverse order
     preRequests.addPending({ requestId: '1234', url: 'foo', method: 'WRONGMETHOD' } as BrowserPreRequest)
     preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
+    const thirdPreRequest = { requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest
+
+    preRequests.addPending(thirdPreRequest)
+    expectPendingCounts(0, 3)
 
     const cb = sinon.stub()
 
     preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
 
     const { args } = cb.getCall(0)
 
-    expect(args[0]).to.include({ requestId: '1234', url: 'foo', method: 'GET' })
+    expect(args[0]).to.eq(thirdPreRequest)
+
+    expectPendingCounts(0, 2)
   })
 
   it('synchronously matches a pre-request added after the request', (done) => {
     const cb = (preRequest) => {
       expect(preRequest).to.include({ requestId: '1234', url: 'foo', method: 'GET' })
+      expectPendingCounts(0, 0)
       done()
     }
 
@@ -41,6 +53,7 @@ describe('http/util/prerequests', () => {
   it('invokes a request callback after a timeout if no pre-request occurs', (done) => {
     const cb = (preRequest) => {
       expect(preRequest).to.be.undefined
+      expectPendingCounts(0, 0)
       done()
     }
 
@@ -57,6 +70,7 @@ describe('http/util/prerequests', () => {
     setTimeout(() => {
       const cb = (preRequest) => {
         expect(preRequest).to.be.undefined
+        expectPendingCounts(0, 0)
         done()
       }