Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: proxy issues with service workers and clean up at end of specs #28060

Merged
merged 22 commits into from
Oct 18, 2023
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/proxy/lib/http/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ type HttpMiddlewareCtx<T> = {
getCookieJar: () => CookieJar
deferSourceMapRewrite: (opts: { js: string, url: string }) => string
getPreRequest: (cb: GetPreRequestCb) => void
addPendingUrlWithoutPreRequest: (url: string) => void
getAUTUrl: Http['getAUTUrl']
setAUTUrl: Http['setAUTUrl']
simulatedCookies: SerializableAutomationCookie[]
Expand Down Expand Up @@ -326,6 +327,9 @@ export class Http {
getPreRequest: (cb) => {
this.preRequests.get(ctx.req, ctx.debug, cb)
},
addPendingUrlWithoutPreRequest: (url) => {
this.preRequests.addPendingUrlWithoutPreRequest(url)
},
protocolManager: this.protocolManager,
}

Expand Down Expand Up @@ -411,6 +415,7 @@ export class Http {
reset () {
this.buffers.reset()
this.setAUTUrl(undefined)
this.preRequests.reset()
}

setBuffer (buffer) {
Expand All @@ -433,4 +438,8 @@ export class Http {
this.protocolManager = protocolManager
this.preRequests.setProtocolManager(protocolManager)
}

setPreRequestTimeout (timeout: number) {
this.preRequests.setPreRequestTimeout(timeout)
}
}
3 changes: 2 additions & 1 deletion packages/proxy/lib/http/request-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -128,8 +128,9 @@ const CorrelateBrowserPreRequest: RequestMiddleware = async function () {
}

this.debug('waiting for prerequest')
this.getPreRequest(((browserPreRequest) => {
this.getPreRequest((({ browserPreRequest, noPreRequestExpected }) => {
this.req.browserPreRequest = browserPreRequest
this.req.noPreRequestExpected = noPreRequestExpected
copyResourceTypeAndNext()
}))
}
Expand Down
4 changes: 4 additions & 0 deletions packages/proxy/lib/http/response-middleware.ts
Original file line number Diff line number Diff line change
Expand Up @@ -656,6 +656,10 @@ const MaybeSendRedirectToClient: ResponseMiddleware = function () {
return this.next()
}

if (this.req.noPreRequestExpected) {
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved
this.addPendingUrlWithoutPreRequest(newUrl)
}

setInitialCookie(this.res, this.remoteStates.current(), true)

this.debug('redirecting to new url %o', { statusCode, newUrl })
Expand Down
88 changes: 74 additions & 14 deletions packages/proxy/lib/http/util/prerequests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ process.once('exit', () => {
debug('metrics: %o', metrics)
})

export type GetPreRequestCb = (browserPreRequest?: BrowserPreRequestWithTimings) => void
export type CorrelationInformation = {
browserPreRequest?: BrowserPreRequestWithTimings
noPreRequestExpected?: boolean
}

export type GetPreRequestCb = (correlationInformation: CorrelationInformation) => void

type PendingRequest = {
ctxDebug
callback: GetPreRequestCb
callback?: GetPreRequestCb
timeout: NodeJS.Timeout
timedOut?: boolean
proxyRequestReceivedTimestamp: number
Expand Down Expand Up @@ -74,6 +79,13 @@ class QueueMap<T> {
this.queues[queueKey].splice(i, 1)
if (this.queues[queueKey].length === 0) delete this.queues[queueKey]
}

forEach (fn: (value: T) => void) {
Object.values(this.queues).forEach((queue) => {
queue.forEach(fn)
})
}

get length () {
return Object.values(this.queues).reduce((prev, cur) => prev + cur.length, 0)
}
Expand Down Expand Up @@ -148,11 +160,16 @@ export class PreRequests {
debugVerbose('Incoming pre-request %s matches pending request. %o', key, browserPreRequest)
if (!pendingRequest.timedOut) {
clearTimeout(pendingRequest.timeout)
pendingRequest.callback({
...browserPreRequest,
...timings,
pendingRequest.callback?.({
browserPreRequest: {
...browserPreRequest,
...timings,
},
noPreRequestExpected: false,
})

delete pendingRequest.callback

return
}

Expand All @@ -179,7 +196,11 @@ export class PreRequests {
if (pendingRequest) {
debugVerbose('Handling %s without a CDP prerequest', key)
clearTimeout(pendingRequest.timeout)
pendingRequest.callback()
pendingRequest.callback?.({
noPreRequestExpected: true,
})

delete pendingRequest.callback

return
}
Expand All @@ -196,6 +217,16 @@ export class PreRequests {
}

get (req: CypressIncomingRequest, ctxDebug, callback: GetPreRequestCb) {
if (req.headers['sec-fetch-dest'] === 'serviceworker') {
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved
ryanthemanuel marked this conversation as resolved.
Show resolved Hide resolved
ctxDebug('Ignoring request with sec-fetch-dest: serviceworker', req.proxiedUrl)

callback({
noPreRequestExpected: true,
})

return
}

const proxyRequestReceivedTimestamp = performance.now() + performance.timeOrigin

metrics.proxyRequestsReceived++
Expand All @@ -206,12 +237,15 @@ export class PreRequests {
metrics.immediatelyMatchedRequests++
ctxDebug('Incoming request %s matches known pre-request: %o', key, pendingPreRequest)
callback({
...pendingPreRequest.browserPreRequest,
cdpRequestWillBeSentTimestamp: pendingPreRequest.cdpRequestWillBeSentTimestamp,
cdpRequestWillBeSentReceivedTimestamp: pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp,
proxyRequestReceivedTimestamp,
cdpLagDuration: pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp - pendingPreRequest.cdpRequestWillBeSentTimestamp,
proxyRequestCorrelationDuration: Math.max(pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp - proxyRequestReceivedTimestamp, 0),
browserPreRequest: {
...pendingPreRequest.browserPreRequest,
cdpRequestWillBeSentTimestamp: pendingPreRequest.cdpRequestWillBeSentTimestamp,
cdpRequestWillBeSentReceivedTimestamp: pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp,
proxyRequestReceivedTimestamp,
cdpLagDuration: pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp - pendingPreRequest.cdpRequestWillBeSentTimestamp,
proxyRequestCorrelationDuration: Math.max(pendingPreRequest.cdpRequestWillBeSentReceivedTimestamp - proxyRequestReceivedTimestamp, 0),
},
noPreRequestExpected: false,
})

return
Expand All @@ -222,7 +256,9 @@ export class PreRequests {
if (pendingUrlWithoutPreRequests) {
metrics.immediatelyMatchedRequests++
ctxDebug('Incoming request %s matches known pending url without pre request', key)
callback()
callback({
noPreRequestExpected: true,
})

return
}
Expand All @@ -235,7 +271,11 @@ export class PreRequests {
ctxDebug('Never received pre-request or url without pre-request for request %s after waiting %sms. Continuing without one.', key, this.requestTimeout)
metrics.unmatchedRequests++
pendingRequest.timedOut = true
callback()
callback({
noPreRequestExpected: false,
})

delete pendingRequest.callback
}, this.requestTimeout),
}

Expand All @@ -245,4 +285,24 @@ export class PreRequests {
setProtocolManager (protocolManager: ProtocolManagerShape) {
this.protocolManager = protocolManager
}

setPreRequestTimeout (requestTimeout: number) {
this.requestTimeout = requestTimeout
}

reset () {
this.pendingPreRequests = new QueueMap<PendingPreRequest>()

// Clear out the pending requests timeout callbacks first then clear the queue
this.pendingRequests.forEach(({ callback, timeout }) => {
clearTimeout(timeout)
metrics.unmatchedRequests++
callback?.({
noPreRequestExpected: false,
})
})

this.pendingRequests = new QueueMap<PendingRequest>()
this.pendingUrlsWithoutPreRequests = new QueueMap<PendingUrlWithoutPreRequest>()
}
}
4 changes: 4 additions & 0 deletions packages/proxy/lib/network-proxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,4 +53,8 @@ export class NetworkProxy {
setProtocolManager (protocolManager) {
this.http.setProtocolManager(protocolManager)
}

setPreRequestTimeout (timeout) {
this.http.setPreRequestTimeout(timeout)
}
}
1 change: 1 addition & 0 deletions packages/proxy/lib/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ export type CypressIncomingRequest = Request & {
abort: () => void
requestId: string
browserPreRequest?: BrowserPreRequestWithTimings
noPreRequestExpected?: boolean
body?: string
responseTimeout?: number
followRedirect?: boolean
Expand Down
94 changes: 65 additions & 29 deletions packages/proxy/test/unit/http/util/prerequests.spec.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { PreRequests } from '@packages/proxy/lib/http/util/prerequests'
import { CorrelationInformation, PreRequests } from '@packages/proxy/lib/http/util/prerequests'
import { BrowserPreRequest, CypressIncomingRequest } from '@packages/proxy'
import { expect } from 'chai'
import sinon from 'sinon'
Expand Down Expand Up @@ -68,22 +68,25 @@ describe('http/util/prerequests', () => {

const cb = sinon.stub()

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)

const { args } = cb.getCall(0)
const arg = args[0]

expect(arg.requestId).to.eq(secondPreRequest.requestId)
expect(arg.url).to.eq(secondPreRequest.url)
expect(arg.method).to.eq(secondPreRequest.method)
expect(arg.headers).to.deep.eq(secondPreRequest.headers)
expect(arg.resourceType).to.eq(secondPreRequest.resourceType)
expect(arg.originalResourceType).to.eq(secondPreRequest.originalResourceType)
expect(arg.cdpRequestWillBeSentTimestamp).to.eq(secondPreRequest.cdpRequestWillBeSentTimestamp)
expect(arg.cdpRequestWillBeSentReceivedTimestamp).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp)
expect(arg.proxyRequestReceivedTimestamp).to.be.a('number')
expect(arg.cdpLagDuration).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp - secondPreRequest.cdpRequestWillBeSentTimestamp)
expect(arg.proxyRequestCorrelationDuration).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp - arg.proxyRequestReceivedTimestamp)
const browserPreRequest = args[0].browserPreRequest
const noPreRequestExpected = args[0].noPreRequestExpected

expect(browserPreRequest.requestId).to.eq(secondPreRequest.requestId)
expect(browserPreRequest.url).to.eq(secondPreRequest.url)
expect(browserPreRequest.method).to.eq(secondPreRequest.method)
expect(browserPreRequest.headers).to.deep.eq(secondPreRequest.headers)
expect(browserPreRequest.resourceType).to.eq(secondPreRequest.resourceType)
expect(browserPreRequest.originalResourceType).to.eq(secondPreRequest.originalResourceType)
expect(browserPreRequest.cdpRequestWillBeSentTimestamp).to.eq(secondPreRequest.cdpRequestWillBeSentTimestamp)
expect(browserPreRequest.cdpRequestWillBeSentReceivedTimestamp).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp)
expect(browserPreRequest.proxyRequestReceivedTimestamp).to.be.a('number')
expect(browserPreRequest.cdpLagDuration).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp - secondPreRequest.cdpRequestWillBeSentTimestamp)
expect(browserPreRequest.proxyRequestCorrelationDuration).to.eq(secondPreRequest.cdpRequestWillBeSentReceivedTimestamp - browserPreRequest.proxyRequestReceivedTimestamp)

expect(noPreRequestExpected).to.be.false

expectPendingCounts(0, 2)
})
Expand All @@ -98,43 +101,47 @@ describe('http/util/prerequests', () => {

const cb = sinon.stub()

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)

const { args } = cb.getCall(0)
const arg = args[0]

expect(arg).to.be.undefined
expect(arg.preRequest).to.be.undefined
expect(arg.noPreRequestExpected).to.be.true

expectPendingCounts(0, 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' })
const cb = ({ browserPreRequest, noPreRequestExpected }: CorrelationInformation) => {
expect(browserPreRequest).to.include({ requestId: '1234', url: 'foo', method: 'GET' })
expect(noPreRequestExpected).to.be.false
expectPendingCounts(0, 0)
done()
}

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)
preRequests.addPending({ requestId: '1234', url: 'foo', method: 'GET' } as BrowserPreRequest)
})

it('synchronously matches a request without a pre-request added after the request', (done) => {
const cb = (preRequest) => {
expect(preRequest).to.be.undefined
const cb = ({ browserPreRequest, noPreRequestExpected }: CorrelationInformation) => {
expect(browserPreRequest).to.be.undefined
expect(noPreRequestExpected).to.be.true
expectPendingCounts(0, 0)
done()
}

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)
preRequests.addPendingUrlWithoutPreRequest('foo')
})

it('invokes a request callback after a timeout if no pre-request occurs', async () => {
let cb
const cbPromise = new Promise<void>((resolve) => {
cb = (preRequest) => {
expect(preRequest).to.be.undefined
cb = ({ browserPreRequest, noPreRequestExpected }: CorrelationInformation) => {
expect(browserPreRequest).to.be.undefined
expect(noPreRequestExpected).to.be.false

// we should have keep the pending request to eventually be correlated later, but don't block the body in the meantime
expectPendingCounts(1, 0)
Expand All @@ -143,7 +150,7 @@ describe('http/util/prerequests', () => {
}
})

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)

await cbPromise

Expand Down Expand Up @@ -182,13 +189,14 @@ describe('http/util/prerequests', () => {
// 2 * requestTimeout. We verify that it's gone (and therefore not leaking memory) by sending in a request
// and assuring that the pre-request wasn't there to be matched anymore.
setTimeout(() => {
const cb = (preRequest) => {
expect(preRequest).to.be.undefined
const cb = ({ browserPreRequest, noPreRequestExpected }: CorrelationInformation) => {
expect(browserPreRequest).to.be.undefined
expect(noPreRequestExpected).to.be.false
expectPendingCounts(1, 0, 0)
done()
}

preRequests.get({ proxiedUrl: 'foo', method: 'GET' } as CypressIncomingRequest, () => {}, cb)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, cb)
}, 1200)
})

Expand Down Expand Up @@ -218,4 +226,32 @@ describe('http/util/prerequests', () => {

expectPendingCounts(0, 2)
})

it('immediately handles a request from a service worker loading', () => {
const cbServiceWorker = sinon.stub()

preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: { 'sec-fetch-dest': 'serviceworker' } } as any, () => {}, cbServiceWorker)

expect(cbServiceWorker).to.be.calledOnce
expect(cbServiceWorker).to.be.calledWith()
})

it('resets the queues', () => {
let callbackCalled = false

preRequests.addPending({ requestId: '1234', url: 'bar', method: 'GET' } as BrowserPreRequest)
preRequests.get({ proxiedUrl: 'foo', method: 'GET', headers: {} } as CypressIncomingRequest, () => {}, () => {
callbackCalled = true
})

preRequests.addPendingUrlWithoutPreRequest('baz')

expectPendingCounts(1, 1, 1)

preRequests.reset()

expectPendingCounts(0, 0, 0)

expect(callbackCalled).to.be.true
})
})
Loading
Loading