diff --git a/cli/CHANGELOG.md b/cli/CHANGELOG.md index b4093cd1ea97..a20c46342968 100644 --- a/cli/CHANGELOG.md +++ b/cli/CHANGELOG.md @@ -1,4 +1,12 @@ +## 13.3.1 + +_Released 10/03/2023 (PENDING)_ + +**Bugfixes:** + +- Fixed an issue where requests were correlated in the wrong order in the proxy. This could cause an issue where the wrong request is used for `cy.intercept` or assets (e.g. stylesheets or images) may not properly be available in Test Replay. Addressed in [#27892](https://github.com/cypress-io/cypress/pull/27892). + ## 13.3.0 _Released 09/27/2023_ diff --git a/packages/proxy/lib/http/index.ts b/packages/proxy/lib/http/index.ts index fc24fc7b69c8..f6436f9391b9 100644 --- a/packages/proxy/lib/http/index.ts +++ b/packages/proxy/lib/http/index.ts @@ -331,12 +331,14 @@ export class Http { const onError = (error: Error): Promise => { ctx.error = error - if (ctx.req.browserPreRequest) { + if (ctx.req.browserPreRequest && !ctx.req.browserPreRequest.errorHandled) { + ctx.req.browserPreRequest.errorHandled = true // browsers will retry requests in the event of network errors, but they will not send pre-requests, // so try to re-use the current browserPreRequest for the next retry after incrementing the ID. const preRequest = { ...ctx.req.browserPreRequest, requestId: getUniqueRequestId(ctx.req.browserPreRequest.requestId), + errorHandled: false, } ctx.debug('Re-using pre-request data %o', preRequest) diff --git a/packages/proxy/lib/http/util/prerequests.ts b/packages/proxy/lib/http/util/prerequests.ts index d598f9397bb6..2062d66967be 100644 --- a/packages/proxy/lib/http/util/prerequests.ts +++ b/packages/proxy/lib/http/util/prerequests.ts @@ -33,40 +33,40 @@ type PendingPreRequest = { } /** - * Data structure that organizes items with duplicated keys into stacks. + * Data structure that organizes items with duplicated keys into queues. */ -class StackMap { - private stacks: Record> = {} - push (stackKey: string, value: T) { - if (!this.stacks[stackKey]) this.stacks[stackKey] = [] +class QueueMap { + private queues: Record> = {} + push (queueKey: string, value: T) { + if (!this.queues[queueKey]) this.queues[queueKey] = [] - this.stacks[stackKey].push(value) + this.queues[queueKey].push(value) } - pop (stackKey: string): T | undefined { - const stack = this.stacks[stackKey] + shift (queueKey: string): T | undefined { + const queue = this.queues[queueKey] - if (!stack) return + if (!queue) return - const item = stack.pop() + const item = queue.shift() - if (stack.length === 0) delete this.stacks[stackKey] + if (queue.length === 0) delete this.queues[queueKey] 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] + Object.entries(this.queues).forEach(([queueKey, queue]) => { + this.queues[queueKey] = queue.filter(filterFn) + if (this.queues[queueKey].length === 0) delete this.queues[queueKey] }) } - removeExact (stackKey: string, value: T) { - const i = this.stacks[stackKey].findIndex((v) => v === value) + removeExact (queueKey: string, value: T) { + const i = this.queues[queueKey].findIndex((v) => v === value) - this.stacks[stackKey].splice(i, 1) - if (this.stacks[stackKey].length === 0) delete this.stacks[stackKey] + this.queues[queueKey].splice(i, 1) + if (this.queues[queueKey].length === 0) delete this.queues[queueKey] } get length () { - return Object.values(this.stacks).reduce((prev, cur) => prev + cur.length, 0) + return Object.values(this.queues).reduce((prev, cur) => prev + cur.length, 0) } } @@ -79,8 +79,8 @@ class StackMap { export class PreRequests { requestTimeout: number sweepInterval: number - pendingPreRequests = new StackMap() - pendingRequests = new StackMap() + pendingPreRequests = new QueueMap() + pendingRequests = new QueueMap() sweepIntervalTimer: NodeJS.Timeout constructor ( @@ -119,7 +119,7 @@ export class PreRequests { addPending (browserPreRequest: BrowserPreRequest) { metrics.browserPreRequestsReceived++ const key = `${browserPreRequest.method}-${browserPreRequest.url}` - const pendingRequest = this.pendingRequests.pop(key) + const pendingRequest = this.pendingRequests.shift(key) if (pendingRequest) { debugVerbose('Incoming pre-request %s matches pending request. %o', key, browserPreRequest) @@ -145,7 +145,7 @@ export class PreRequests { get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) { metrics.proxyRequestsReceived++ const key = `${req.method}-${req.proxiedUrl}` - const pendingPreRequest = this.pendingPreRequests.pop(key) + const pendingPreRequest = this.pendingPreRequests.shift(key) if (pendingPreRequest) { metrics.immediatelyMatchedRequests++ diff --git a/packages/proxy/lib/types.ts b/packages/proxy/lib/types.ts index a6879a56a814..20384ee211e6 100644 --- a/packages/proxy/lib/types.ts +++ b/packages/proxy/lib/types.ts @@ -59,6 +59,7 @@ export type BrowserPreRequest = { headers: { [key: string]: string | string[] } resourceType: ResourceType originalResourceType: string | undefined + errorHandled?: boolean } /** diff --git a/packages/proxy/test/unit/http/index.spec.ts b/packages/proxy/test/unit/http/index.spec.ts index d528f7379420..af02ab70bf94 100644 --- a/packages/proxy/test/unit/http/index.spec.ts +++ b/packages/proxy/test/unit/http/index.spec.ts @@ -79,6 +79,37 @@ describe('http', function () { }) }) + it('ensures not to create fake pending browser pre requests on multiple errors', function () { + incomingRequest.callsFake(function () { + this.req.browserPreRequest = { + errorHandled: true, + } + + throw new Error('oops') + }) + + error.callsFake(function () { + expect(this.error.message).to.eq('Internal error while proxying "GET url" in 0:\noops') + this.end() + }) + + const http = new Http(httpOpts) + + http.addPendingBrowserPreRequest = sinon.stub() + + return http + // @ts-ignore + .handleHttpRequest({ method: 'GET', proxiedUrl: 'url' }, { on, off }) + .then(function () { + expect(incomingRequest).to.be.calledOnce + expect(incomingResponse).to.not.be.called + expect(http.addPendingBrowserPreRequest).to.not.be.called + expect(error).to.be.calledOnce + expect(on).to.not.be.called + expect(off).to.be.calledThrice + }) + }) + it('moves to Error stack if err in IncomingResponse', function () { incomingRequest.callsFake(function () { this.incomingRes = {} diff --git a/packages/proxy/test/unit/http/util/prerequests.spec.ts b/packages/proxy/test/unit/http/util/prerequests.spec.ts index 08b1fd6a9a4a..f325e539ecf8 100644 --- a/packages/proxy/test/unit/http/util/prerequests.spec.ts +++ b/packages/proxy/test/unit/http/util/prerequests.spec.ts @@ -20,12 +20,12 @@ describe('http/util/prerequests', () => { }) it('synchronously matches a pre-request that existed at the time of the request', () => { - // should match in reverse order + // should match in 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 + const secondPreRequest = { requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest - preRequests.addPending(thirdPreRequest) + preRequests.addPending(secondPreRequest) + preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest) expectPendingCounts(0, 3) const cb = sinon.stub() @@ -34,7 +34,7 @@ describe('http/util/prerequests', () => { const { args } = cb.getCall(0) - expect(args[0]).to.eq(thirdPreRequest) + expect(args[0]).to.eq(secondPreRequest) expectPendingCounts(0, 2) })