Skip to content

Commit

Permalink
fix: disable automatic request retries (#19161)
Browse files Browse the repository at this point in the history
  • Loading branch information
@flotwig
flotwig committed Dec 8, 2021
1 parent d18878f commit 65cf6e8
Show file tree
Hide file tree
Showing 10 changed files with 66 additions and 191 deletions.
1 change: 1 addition & 0 deletions packages/driver/cypress/integration/commands/request_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ describe('src/cy/commands/request', () => {
followRedirect: true,
timeout: RESPONSE_TIMEOUT,
method: 'GET',
retryIntervals: [0, 100, 200, 200],
})

const options = backend.firstCall.args[1]
Expand Down
3 changes: 2 additions & 1 deletion packages/driver/src/cy/commands/navigation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ let hasVisitedAboutBlank = null
let currentlyVisitingAboutBlank = null
let knownCommandCausedInstability = null

const REQUEST_URL_OPTS = 'auth failOnStatusCode retryOnNetworkFailure retryOnStatusCodeFailure method body headers'
const REQUEST_URL_OPTS = 'auth failOnStatusCode retryOnNetworkFailure retryOnStatusCodeFailure retryIntervals method body headers'
.split(' ')

const VISIT_OPTS = 'url log onBeforeLoad onLoad timeout requestTimeout'
Expand Down Expand Up @@ -712,6 +712,7 @@ export default (Commands, Cypress, cy, state, config) => {
failOnStatusCode: true,
retryOnNetworkFailure: true,
retryOnStatusCodeFailure: false,
retryIntervals: [0, 100, 200, 200],
method: 'GET',
body: null,
headers: {},
Expand Down
1 change: 1 addition & 0 deletions packages/driver/src/cy/commands/request.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ const REQUEST_DEFAULTS = {
timeout: null,
followRedirect: true,
failOnStatusCode: true,
retryIntervals: [0, 100, 200, 200],
retryOnNetworkFailure: true,
retryOnStatusCodeFailure: false,
}
Expand Down
2 changes: 1 addition & 1 deletion packages/proxy/lib/http/request-middleware.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ const SendRequestOutgoing: RequestMiddleware = function () {
timeout: this.req.responseTimeout,
strictSSL: false,
followRedirect: this.req.followRedirect || false,
retryIntervals: [0, 100, 200, 200],
retryIntervals: [],
url: this.req.proxiedUrl,
}

Expand Down
4 changes: 2 additions & 2 deletions packages/proxy/lib/http/util/prerequests.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,11 @@ process.once('exit', () => {
})

function removeOne<T> (a: Array<T>, predicate: (v: T) => boolean): T | void {
for (const i in a) {
for (let i = a.length - 1; i >= 0; i--) {
const v = a[i]

if (predicate(v)) {
a.splice(i as unknown as number, 1)
a.splice(i, 1)

return v
}
Expand Down
2 changes: 2 additions & 0 deletions packages/proxy/test/unit/http/util/prerequests.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ describe('http/util/prerequests', () => {
})

it('synchronously matches a pre-request that existed at the time of the request', () => {
// 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 cb = sinon.stub()
Expand Down
53 changes: 29 additions & 24 deletions packages/server/lib/request.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ const getOriginalHeaders = (req = {}) => {
}

const getDelayForRetry = function (options = {}) {
const { err, opts, delaysRemaining, retryIntervals, onNext, onElse } = options
const { err, opts, delaysRemaining, retryIntervals, retryFn, onEnd } = options

let delay = delaysRemaining.shift()

if (!_.isNumber(delay)) {
// no more delays, bailing
debug('exhausted all attempts retrying request %o', merge(opts, { err }))

return onElse()
return onEnd()
}

// figure out which attempt we're on...
Expand All @@ -76,7 +76,7 @@ const getDelayForRetry = function (options = {}) {
attempt,
}))

return onNext(delay, attempt)
return retryFn({ delay, attempt })
}

const hasRetriableStatusCodeFailure = (res, retryOnStatusCodeFailure) => {
Expand Down Expand Up @@ -105,8 +105,8 @@ const maybeRetryOnNetworkFailure = function (err, options = {}) {
retryIntervals,
delaysRemaining,
retryOnNetworkFailure,
onNext,
onElse,
retryFn,
onEnd,
} = options

debug('received an error making http request %o', merge(opts, { err }))
Expand All @@ -118,10 +118,15 @@ const maybeRetryOnNetworkFailure = function (err, options = {}) {
// https://github.com/cypress-io/cypress/pull/6705
debug('detected TLS version error, setting min version to TLSv1')
opts.minVersion = 'TLSv1'

if (retryIntervals.length === 0) {
// normally, this request would not be retried, but we need to retry in order to support TLSv1
return retryFn({ delay: 0, attempt: 1 })
}
}

if (!isTlsVersionError && !isErrEmptyResponseError(err.originalErr || err) && !isRetriableError(err, retryOnNetworkFailure)) {
return onElse()
return onEnd()
}

// else see if we have more delays left...
Expand All @@ -130,8 +135,8 @@ const maybeRetryOnNetworkFailure = function (err, options = {}) {
opts,
retryIntervals,
delaysRemaining,
onNext,
onElse,
retryFn,
onEnd,
})
}

Expand All @@ -143,8 +148,8 @@ const maybeRetryOnStatusCodeFailure = function (res, options = {}) {
retryIntervals,
delaysRemaining,
retryOnStatusCodeFailure,
onNext,
onElse,
retryFn,
onEnd,
} = options

debug('received status code & headers on request %o', {
Expand All @@ -156,7 +161,7 @@ const maybeRetryOnStatusCodeFailure = function (res, options = {}) {
// is this a retryable status code failure?
if (!hasRetriableStatusCodeFailure(res, retryOnStatusCodeFailure)) {
// if not then we're done here
return onElse()
return onEnd()
}

// else see if we have more delays left...
Expand All @@ -165,8 +170,8 @@ const maybeRetryOnStatusCodeFailure = function (res, options = {}) {
opts,
retryIntervals,
delaysRemaining,
onNext,
onElse,
retryFn,
onEnd,
})
}

Expand Down Expand Up @@ -201,7 +206,7 @@ const createRetryingRequestPromise = function (opts) {
retryOnStatusCodeFailure,
} = opts

const retry = (delay) => {
const retry = ({ delay }) => {
return Promise.delay(delay)
.then(() => {
return createRetryingRequestPromise(opts)
Expand All @@ -216,8 +221,8 @@ const createRetryingRequestPromise = function (opts) {
retryIntervals,
delaysRemaining,
retryOnNetworkFailure,
onNext: retry,
onElse () {
retryFn: retry,
onEnd () {
throw err
},
})
Expand All @@ -229,8 +234,8 @@ const createRetryingRequestPromise = function (opts) {
retryIntervals,
delaysRemaining,
retryOnStatusCodeFailure,
onNext: retry,
onElse: _.constant(res),
retryFn: retry,
onEnd: _.constant(res),
})
})
}
Expand Down Expand Up @@ -281,7 +286,7 @@ const createRetryingRequestStream = function (opts = {}) {
const reqStream = r(opts)
let didReceiveResponse = false

const retry = function (delay, attempt) {
const retry = function ({ delay, attempt }) {
retryStream.emit('retry', { attempt, delay })

return setTimeout(tryStartStream, delay)
Expand Down Expand Up @@ -337,8 +342,8 @@ const createRetryingRequestStream = function (opts = {}) {
retryIntervals,
delaysRemaining,
retryOnNetworkFailure,
onNext: retry,
onElse () {
retryFn: retry,
onEnd () {
return emitError(err)
},
})
Expand All @@ -360,8 +365,8 @@ const createRetryingRequestStream = function (opts = {}) {
delaysRemaining,
retryIntervals,
retryOnStatusCodeFailure,
onNext: retry,
onElse () {
retryFn: retry,
onEnd () {
debug('successful response received', { requestId })

cleanup()
Expand Down Expand Up @@ -414,7 +419,7 @@ const setDefaults = (opts) => {
.chain(opts)
.defaults({
requestId: _.uniqueId('request'),
retryIntervals: [0, 1000, 2000, 2000],
retryIntervals: [],
retryOnNetworkFailure: true,
retryOnStatusCodeFailure: false,
})
Expand Down
38 changes: 19 additions & 19 deletions packages/server/test/unit/request_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,24 +66,24 @@ describe('lib/request', () => {
code: 'ECONNREFUSED',
}

const onNext = sinon.stub()
const retryFn = sinon.stub()

retryIntervals.forEach(() => {
return request.getDelayForRetry({
err,
onNext,
retryFn,
retryIntervals,
delaysRemaining,
})
})

expect(delaysRemaining).to.be.empty

expect(onNext.args).to.deep.eq([
[0, 1],
[999, 2],
[100, 3],
[200, 4],
expect(retryFn.args).to.deep.eq([
[{ delay: 0, attempt: 1 }],
[{ delay: 999, attempt: 2 }],
[{ delay: 100, attempt: 3 }],
[{ delay: 200, attempt: 4 }],
])
})

Expand All @@ -95,37 +95,37 @@ describe('lib/request', () => {
code: 'ECONNRESET',
}

const onNext = sinon.stub()
const retryFn = sinon.stub()

request.getDelayForRetry({
err,
onNext,
retryFn,
retryIntervals,
delaysRemaining,
})

expect(delaysRemaining).to.have.length(3)

expect(onNext).to.be.calledWith(2000, 1)
expect(retryFn).to.be.calledWith({ delay: 2000, attempt: 1 })
})

it('calls onElse when delaysRemaining is exhausted', () => {
it('calls onEnd when delaysRemaining is exhausted', () => {
const retryIntervals = [1, 2, 3, 4]
const delaysRemaining = []

const onNext = sinon.stub()
const onElse = sinon.stub()
const retryFn = sinon.stub()
const onEnd = sinon.stub()

request.getDelayForRetry({
onElse,
onNext,
onEnd,
retryFn,
retryIntervals,
delaysRemaining,
})

expect(onElse).to.be.calledWithExactly()
expect(onEnd).to.be.calledWithExactly()

expect(onNext).not.to.be.called
expect(retryFn).not.to.be.called
})
})

Expand All @@ -141,10 +141,10 @@ describe('lib/request', () => {
expect(opts.delaysRemaining).to.deep.eq(retryIntervals)
})

it('retryIntervals to [0, 1000, 2000, 2000] by default', () => {
it('retryIntervals to [] by default', () => {
const opts = request.setDefaults({})

expect(opts.retryIntervals).to.deep.eq([0, 1000, 2000, 2000])
expect(opts.retryIntervals).to.deep.eq([])
})

it('delaysRemaining can be overridden', () => {
Expand Down
62 changes: 0 additions & 62 deletions system-tests/__snapshots__/network_error_handling_spec.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,68 +15,6 @@ Cypress failed to verify that your server is running.
Please start this server and then run Cypress again.
`

exports['e2e network error handling Cypress retries HTTPS passthrough behind a proxy 1'] = `
====================================================================================================
(Run Starting)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Cypress: 1.2.3 │
│ Browser: FooBrowser 88 │
│ Specs: 1 found (https_passthru_spec.js) │
│ Searched: cypress/integration/https_passthru_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
────────────────────────────────────────────────────────────────────────────────────────────────────
Running: https_passthru_spec.js (1 of 1)
https passthru retries
✓ retries when visiting a non-test domain
✓ passes through the network error when it cannot connect to the proxy
2 passing
(Results)
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ Tests: 2 │
│ Passing: 2 │
│ Failing: 0 │
│ Pending: 0 │
│ Skipped: 0 │
│ Screenshots: 0 │
│ Video: true │
│ Duration: X seconds │
│ Spec Ran: https_passthru_spec.js │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
(Video)
- Started processing: Compressing to 32 CRF
- Finished processing: /XXX/XXX/XXX/cypress/videos/https_passthru_spec.js.mp4 (X second)
====================================================================================================
(Run Finished)
Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ https_passthru_spec.js XX:XX 2 2 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! XX:XX 2 2 - - -
`

exports['e2e network error handling Cypress does not connect to the upstream proxy for the SNI server request 1'] = `
Expand Down

3 comments on commit 65cf6e8

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 65cf6e8 Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the linux x64 version of the Test Runner.

Learn more about this pre-release platform-specific build at https://on.cypress.io/installing-cypress#Install-pre-release-version.

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/9.1.2/circle-develop-65cf6e899d95561e35cd39651f98546488908f67/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 65cf6e8 Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the win32 x64 version of the Test Runner.

Learn more about this pre-release platform-specific build at https://on.cypress.io/installing-cypress#Install-pre-release-version.

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/9.1.2/circle-develop-65cf6e899d95561e35cd39651f98546488908f67/cypress.tgz

@cypress-bot
Copy link
Contributor

@cypress-bot cypress-bot bot commented on 65cf6e8 Dec 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Circle has built the darwin x64 version of the Test Runner.

Learn more about this pre-release platform-specific build at https://on.cypress.io/installing-cypress#Install-pre-release-version.

Run this command to install the pre-release locally:

npm install https://cdn.cypress.io/beta/npm/9.1.2/circle-develop-65cf6e899d95561e35cd39651f98546488908f67/cypress.tgz

Please sign in to comment.