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
7 changes: 6 additions & 1 deletion packages/server/lib/request.js
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,11 @@ 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 onNext(0, 1)
Copy link
Member

Choose a reason for hiding this comment

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

what does onNext(0, 1) do? Is there a way we could make the code more descriptive?

Copy link
Contributor Author

@flotwig flotwig Dec 8, 2021

Choose a reason for hiding this comment

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

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

So this runs a single retry instantly.

Is there a way we could make the code more descriptive?

So many ways. 😄 I started refactoring this file and it became a much bigger task, so I decided to keep this PR to the bare minimum changes. For now I pushed a commit to clean up onNext/onElse specifically: d618df3

}
}

if (!isTlsVersionError && !isErrEmptyResponseError(err.originalErr || err) && !isRetriableError(err, retryOnNetworkFailure)) {
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
4 changes: 2 additions & 2 deletions packages/server/test/unit/request_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
91 changes: 9 additions & 82 deletions system-tests/test/network_error_handling_spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ let mitmProxy = require('http-mitm-proxy')
const PORT = 13370
const PROXY_PORT = 13371
const HTTPS_PORT = 13372
const ERR_HTTPS_PORT = 13373

const start = Number(new Date())

Expand Down Expand Up @@ -382,21 +381,23 @@ describe('e2e network error handling', function () {
expectedExitCode: 2,
snapshot: true,
}).then(({ stdout }) => {
// sometimes <img>, <script> get retried 2x by chrome instead of 1x
// sometimes <img>, <script> get retried, sometimes they do not

if (counts['/immediate-reset?load-img'] === 10) {
counts['/immediate-reset?load-img'] = 5
if (counts['/immediate-reset?load-img'] > 1) {
console.log('load-img was retried', counts['/immediate-reset?load-img'], 'times')
counts['/immediate-reset?load-img'] = 1
}

if (counts['/immediate-reset?load-js'] === 10) {
counts['/immediate-reset?load-js'] = 5
if (counts['/immediate-reset?load-js'] > 1) {
console.log('load-js was retried', counts['/immediate-reset?load-js'], 'times')
counts['/immediate-reset?load-js'] = 1
}

expect(counts).to.deep.eq({
'/immediate-reset?visit': 5,
'/immediate-reset?request': 5,
'/immediate-reset?load-img': 5,
'/immediate-reset?load-js': 5,
'/immediate-reset?load-img': 1,
'/immediate-reset?load-js': 1,
'/works-third-time-else-500/500-for-request': 3,
'/works-third-time/for-request': 3,
'/works-third-time-else-500/500-for-visit': 3,
Expand All @@ -409,80 +410,6 @@ describe('e2e network error handling', function () {
})
})

it('retries HTTPS passthrough behind a proxy', function () {
// this tests retrying multiple times
// to connect to the upstream server
// as well as network errors when the
// upstream server is not accessible

const connectCounts = {}

const onConnect = function ({ host, port, socket }) {
const dest = `${host}:${port}`

if (connectCounts[dest] == null) {
connectCounts[dest] = 0
}

connectCounts[dest] += 1

switch (port) {
case HTTPS_PORT:
// this tests network related errors
// when we do immediately destroy the
// socket and prevent connecting to the
// upstream server
//
// on the 3rd time around, don't destroy the socket.
if (connectCounts[`localhost:${HTTPS_PORT}`] >= 3) {
return true
}

// else if this is the 1st or 2nd time destroy the
// socket so we retry connecting to the debug proxy
socket.destroy()

return false

case ERR_HTTPS_PORT:
// always destroy the socket attempting to connect
// to the upstream server to test that network errors
// are propagated correctly
socket.destroy()

return false

default:
// pass everything else on to the upstream
// server as expected
return true
}
}

this.debugProxy = new DebugProxy({
onConnect,
})

return this.debugProxy
.start(PROXY_PORT)
.then(() => {
process.env.HTTP_PROXY = `http://localhost:${PROXY_PORT}`
process.env.NO_PROXY = '<-loopback>' // proxy everything including localhost

return systemTests.exec(this, {
spec: 'https_passthru_spec.js',
snapshot: true,
})
.then(() => {
console.log('connect counts are', connectCounts)

expect(connectCounts[`localhost:${HTTPS_PORT}`]).to.be.gte(3)

expect(connectCounts[`localhost:${ERR_HTTPS_PORT}`]).to.be.gte(4)
})
})
})

it('does not connect to the upstream proxy for the SNI server request', function () {
const onConnect = sinon.spy(() => {
return true
Expand Down