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: disable automatic request retries #19161

Merged
merged 12 commits into from
Dec 8, 2021
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
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
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],
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding retry intervals here and above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously this would inherit from the defaults in server/lib/request, but now it has to set its own.

Automatic retries are still used for cy.visit's resolve:url and cy.request, since those are not retriable by the browser. Fully removing automatic retries is a breaking change b/c we expose retryOnNetworkFailure and retryOnStatusCodeFailure as opts on cy.request, cy.visit

retryOnNetworkFailure: true,
retryOnStatusCodeFailure: false,
}
Expand Down
2 changes: 1 addition & 1 deletion packages/proxy/lib/http/request-middleware.ts
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
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
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
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 }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 to destructuring refactor

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
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
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