diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index 79cfe638b208..0b7dfbe0a425 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -5,6 +5,7 @@ _Released 10/24/2023 (PENDING)_ **Bugfixes:** +- Fixed a performance problem with proxy correlation when requests get aborted and then get miscorrelated with follow up requests. Fixed in [#28094](/~https://github.com/cypress-io/cypress/pull/28094). - Fixed a regression in [10.0.0](#10.0.0), where search would not find a spec if the file name contains "-" or "\_", but search prompt contains " " instead (e.g. search file "spec-file.cy.ts" with prompt "spec file"). Fixes [#25303](/~https://github.com/cypress-io/cypress/issues/25303). ## 13.3.2 diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index 3ccb9e80afbf..7549e508e67e 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -9,7 +9,7 @@ import ErrorMiddleware from './error-middleware' import RequestMiddleware from './request-middleware' import ResponseMiddleware from './response-middleware' import { HttpBuffers } from './util/buffers' -import { GetPreRequestCb, PreRequests } from './util/prerequests' +import { GetPreRequestCb, PendingRequest, PreRequests } from './util/prerequests' import type EventEmitter from 'events' import type CyServer from '@packages/server' @@ -63,10 +63,12 @@ type HttpMiddlewareCtx = { stage: HttpStages debug: Debug.Debugger middleware: HttpMiddlewareStacks + pendingRequest: PendingRequest | undefined getCookieJar: () => CookieJar deferSourceMapRewrite: (opts: { js: string, url: string }) => string - getPreRequest: (cb: GetPreRequestCb) => void + getPreRequest: (cb: GetPreRequestCb) => PendingRequest | undefined addPendingUrlWithoutPreRequest: (url: string) => void + removePendingRequest: (pendingRequest: PendingRequest) => void getAUTUrl: Http['getAUTUrl'] setAUTUrl: Http['setAUTUrl'] simulatedCookies: SerializableAutomationCookie[] @@ -325,15 +327,25 @@ export class Http { getAUTUrl: this.getAUTUrl, setAUTUrl: this.setAUTUrl, getPreRequest: (cb) => { - this.preRequests.get(ctx.req, ctx.debug, cb) + return this.preRequests.get(ctx.req, ctx.debug, cb) }, addPendingUrlWithoutPreRequest: (url) => { this.preRequests.addPendingUrlWithoutPreRequest(url) }, + removePendingRequest: (pendingRequest: PendingRequest) => { + this.preRequests.removePendingRequest(pendingRequest) + }, protocolManager: this.protocolManager, } const onError = (error: Error): Promise => { + const pendingRequest = ctx.pendingRequest as PendingRequest | undefined + + if (pendingRequest) { + delete ctx.pendingRequest + ctx.removePendingRequest(pendingRequest) + } + ctx.error = error if (ctx.req.browserPreRequest && !ctx.req.browserPreRequest.errorHandled) { ctx.req.browserPreRequest.errorHandled = true @@ -427,7 +439,7 @@ export class Http { } removePendingBrowserPreRequest (requestId: string) { - this.preRequests.removePending(requestId) + this.preRequests.removePendingPreRequest(requestId) } addPendingUrlWithoutPreRequest (url: string) { diff --git a/packages/proxy/lib/http/request-middleware.ts b/packages/proxy/lib/http/request-middleware.ts index 04391b5fadfe..08d4490a1c33 100644 --- a/packages/proxy/lib/http/request-middleware.ts +++ b/packages/proxy/lib/http/request-middleware.ts @@ -128,7 +128,7 @@ const CorrelateBrowserPreRequest: RequestMiddleware = async function () { } this.debug('waiting for prerequest') - this.getPreRequest((({ browserPreRequest, noPreRequestExpected }) => { + this.pendingRequest = this.getPreRequest((({ browserPreRequest, noPreRequestExpected }) => { this.req.browserPreRequest = browserPreRequest this.req.noPreRequestExpected = noPreRequestExpected copyResourceTypeAndNext() @@ -455,6 +455,13 @@ const SendRequestOutgoing: RequestMiddleware = function () { this.debug('request aborted') // if the request is aborted, close out the middleware span and http span. the response middleware did not run + const pendingRequest = this.pendingRequest + + if (pendingRequest) { + delete this.pendingRequest + this.removePendingRequest(pendingRequest) + } + this.reqMiddlewareSpan?.setAttributes({ requestAborted: true, }) diff --git a/packages/proxy/lib/http/util/prerequests.ts b/packages/proxy/lib/http/util/prerequests.ts index 9759828af211..cbdd41f9a13f 100644 --- a/packages/proxy/lib/http/util/prerequests.ts +++ b/packages/proxy/lib/http/util/prerequests.ts @@ -28,7 +28,8 @@ export type CorrelationInformation = { export type GetPreRequestCb = (correlationInformation: CorrelationInformation) => void -type PendingRequest = { +export type PendingRequest = { + key: string ctxDebug callback?: GetPreRequestCb timeout: NodeJS.Timeout @@ -74,10 +75,12 @@ class QueueMap { }) } removeExact (queueKey: string, value: T) { - const i = this.queues[queueKey].findIndex((v) => v === value) + const i = this.queues[queueKey]?.findIndex((v) => v === value) - this.queues[queueKey].splice(i, 1) - if (this.queues[queueKey].length === 0) delete this.queues[queueKey] + if (i > -1) { + this.queues[queueKey].splice(i, 1) + if (this.queues[queueKey].length === 0) delete this.queues[queueKey] + } } forEach (fn: (value: T) => void) { @@ -210,7 +213,7 @@ export class PreRequests { }) } - removePending (requestId: string) { + removePendingPreRequest (requestId: string) { this.pendingPreRequests.removeMatching(({ browserPreRequest }) => { return (browserPreRequest.requestId.includes('-retry-') && !browserPreRequest.requestId.startsWith(`${requestId}-`)) || (!browserPreRequest.requestId.includes('-retry-') && browserPreRequest.requestId !== requestId) }) @@ -267,6 +270,7 @@ export class PreRequests { } const pendingRequest: PendingRequest = { + key, ctxDebug, callback, proxyRequestReceivedTimestamp: performance.now() + performance.timeOrigin, @@ -283,6 +287,8 @@ export class PreRequests { } this.pendingRequests.push(key, pendingRequest) + + return pendingRequest } setProtocolManager (protocolManager: ProtocolManagerShape) { @@ -293,6 +299,12 @@ export class PreRequests { this.requestTimeout = requestTimeout } + removePendingRequest (pendingRequest: PendingRequest) { + this.pendingRequests.removeExact(pendingRequest.key, pendingRequest) + clearTimeout(pendingRequest.timeout) + delete pendingRequest.callback + } + reset () { this.pendingPreRequests = new QueueMap() diff --git a/packages/proxy/test/unit/http/util/prerequests.spec.ts b/packages/proxy/test/unit/http/util/prerequests.spec.ts index f607a823ee0b..391a73ef957d 100644 --- a/packages/proxy/test/unit/http/util/prerequests.spec.ts +++ b/packages/proxy/test/unit/http/util/prerequests.spec.ts @@ -207,7 +207,7 @@ describe('http/util/prerequests', () => { expectPendingCounts(0, 3) - preRequests.removePending('1235') + preRequests.removePendingPreRequest('1235') expectPendingCounts(0, 2) }) @@ -222,7 +222,7 @@ describe('http/util/prerequests', () => { expectPendingCounts(0, 6) - preRequests.removePending('1235') + preRequests.removePendingPreRequest('1235') expectPendingCounts(0, 2) }) @@ -236,6 +236,27 @@ describe('http/util/prerequests', () => { expect(cbServiceWorker).to.be.calledWith() }) + it('removes a pending request', () => { + const cb = sinon.stub() + + const firstPreRequest = preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb) + const secondPreRequest = preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb) + + expectPendingCounts(2, 0) + + preRequests.removePendingRequest(firstPreRequest!) + + expectPendingCounts(1, 0) + + preRequests.removePendingRequest(firstPreRequest!) + + expectPendingCounts(1, 0) + + preRequests.removePendingRequest(secondPreRequest!) + + expectPendingCounts(0, 0) + }) + it('resets the queues', () => { let callbackCalled = false diff --git a/packages/server/test/integration/http_requests_spec.js b/packages/server/test/integration/http_requests_spec.js index 921ed2097edc..5eed3127763b 100644 --- a/packages/server/test/integration/http_requests_spec.js +++ b/packages/server/test/integration/http_requests_spec.js @@ -94,7 +94,7 @@ describe('Routes', () => { Fixtures.scaffold() - this.setup = async (initialUrl, obj = {}, spec) => { + this.setup = async (initialUrl, obj = {}, spec, shouldCorrelatePreRequests = false) => { if (_.isObject(initialUrl)) { obj = initialUrl initialUrl = null @@ -170,6 +170,7 @@ describe('Routes', () => { createRoutes, testingType: 'e2e', exit: false, + shouldCorrelatePreRequests: () => shouldCorrelatePreRequests, }) .spread(async (port) => { const automationStub = { @@ -187,6 +188,8 @@ describe('Routes', () => { this.session = session(this.srv) this.proxy = `http://localhost:${port}` + + this.networkProxy = this.server._networkProxy }), ]) }) @@ -983,6 +986,108 @@ describe('Routes', () => { }) }) + context('basic request with correlation', () => { + beforeEach(function () { + return this.setup('http://www.github.com', undefined, undefined, true) + }) + + it('properly correlates when CDP failures come first', function () { + this.timeout(1500) + + nock(this.server.remoteStates.current().origin) + .get('/') + .reply(200, 'hello from bar!', { + 'Content-Type': 'text/html', + }) + + this.networkProxy.addPendingBrowserPreRequest({ + requestId: '1', + method: 'GET', + url: 'http://www.github.com/', + }) + + this.networkProxy.removePendingBrowserPreRequest({ + requestId: '1', + }) + + const requestPromise = this.rp({ + url: 'http://www.github.com/', + headers: { + 'Cookie': '__cypress.initial=true', + 'Accept-Encoding': 'identity', + }, + }) + + this.networkProxy.addPendingBrowserPreRequest({ + requestId: '1', + method: 'GET', + url: 'http://www.github.com/', + }) + + return requestPromise.then((res) => { + expect(res.statusCode).to.eq(200) + + expect(res.body).to.include('hello from bar!') + }) + }) + + it('properly correlates when proxy failure come first', function () { + this.networkProxy.setPreRequestTimeout(50) + // If this takes longer than the Promise.delay and the prerequest timeout then the second + // call has hit the prerequest timeout which is a problem + this.timeout(150) + + nock(this.server.remoteStates.current().origin) + .get('/') + .delay(50) + .once() + .reply(200, 'hello from bar!', { + 'Content-Type': 'text/html', + }) + + this.rp({ + url: 'http://www.github.com/', + headers: { + 'Cookie': '__cypress.initial=true', + 'Accept-Encoding': 'identity', + }, + // Timeout needs to be less than the prerequest timeout + the nock delay + timeout: 75, + }).catch(() => {}) + + // Wait 100 ms to make sure the request times out + return Promise.delay(100).then(() => { + nock(this.server.remoteStates.current().origin) + .get('/') + .once() + .reply(200, 'hello from baz!', { + 'Content-Type': 'text/html', + }) + + // This should not immediately correlate. If it does, then the next request will timeout + this.networkProxy.addPendingBrowserPreRequest({ + requestId: '1', + method: 'GET', + url: 'http://www.github.com/', + }) + + const followupRequestPromise = this.rp({ + url: 'http://www.github.com/', + headers: { + 'Cookie': '__cypress.initial=true', + 'Accept-Encoding': 'identity', + }, + }) + + return followupRequestPromise.then((res) => { + expect(res.statusCode).to.eq(200) + + expect(res.body).to.include('hello from baz!') + }) + }) + }) + }) + context('gzip', () => { beforeEach(function () { return this.setup('http://www.github.com')