diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 000000000..e4ddae084 --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,34 @@ +name: test + +on: + push: + branches: [v11] + pull_request: + branches: [v11] + +env: + DEPENDENCIES_BEFORE: '2021-06-15' + TYPES_NODE_VERSION: '14.18.34' + +jobs: + test: + name: Node.js ${{ matrix.node-version }} + runs-on: ubuntu-latest + strategy: + fail-fast: false + matrix: + node-version: [lts/*, latest] + steps: + - uses: actions/checkout@v7 + - uses: actions/setup-node@v7 + with: + node-version: ${{ matrix.node-version }} + - run: node --version + - name: Install + run: | + npm install --ignore-scripts --before=$DEPENDENCIES_BEFORE + npm install --ignore-scripts --no-save @types/node@$TYPES_NODE_VERSION + - name: Build + run: npm run build + - name: Test + run: npx ava test/retry.ts diff --git a/source/core/index.ts b/source/core/index.ts index bec7192fc..389c61e46 100644 --- a/source/core/index.ts +++ b/source/core/index.ts @@ -2696,14 +2696,18 @@ export default class Request extends Duplex implements RequestEvents { } this[kRequest]!.end((error?: Error | null) => { - if (!error) { - this[kBodySize] = this[kUploadedSize]; - - this.emit('uploadProgress', this.uploadProgress); - this[kRequest]!.emit('upload-complete'); + if (error) { + // `ClientRequest.end()` can report the same failure as the request's `error` event. Route it through Got's retry handling without completing `_final`, so this Duplex does not finish a failed upload. + this._beforeError(error); + return; } - callback(error); + this[kBodySize] = this[kUploadedSize]; + + this.emit('uploadProgress', this.uploadProgress); + this[kRequest]!.emit('upload-complete'); + + callback(); }); }; diff --git a/test/retry.ts b/test/retry.ts index ba7ac2318..885a9765d 100644 --- a/test/retry.ts +++ b/test/retry.ts @@ -1,13 +1,13 @@ import {EventEmitter} from 'events'; import {PassThrough as PassThroughStream} from 'stream'; -import {Socket} from 'net'; +import {Socket, createServer} from 'net'; import http = require('http'); import test from 'ava'; import is from '@sindresorhus/is'; import {Handler} from 'express'; import getStream = require('get-stream'); import pEvent = require('p-event'); -import got, {HTTPError} from '../source'; +import got, {HTTPError, RequestError} from '../source'; import withServer from './helpers/with-server'; const retryAfterOn413 = 2; @@ -20,6 +20,52 @@ const handler413: Handler = (_request, response) => { response.end(); }; +type RequestEndErrorScenario = 'request-error-first' | 'end-callback-only'; + +const createRequestWithEndError = (scenario: RequestEndErrorScenario): http.ClientRequest => { + const request = new EventEmitter() as http.ClientRequest; + + // @ts-expect-error Mocking the behaviour of a ClientRequest + request.end = (callback: (error: Error) => void) => { + const connectionError = Object.assign(new Error('connect ECONNREFUSED 127.0.0.1:80'), {code: 'ECONNREFUSED'}); + + process.nextTick(() => { + if (scenario === 'request-error-first') { + request.emit('error', connectionError); + } + + callback(connectionError); + }); + }; + + request.abort = () => {}; + + request.destroy = () => { + request.destroyed = true; + return request; + }; + + return request; +}; + +const retryImmediately = { + calculateDelay: ({computedValue}: {computedValue: number}) => computedValue === 0 ? 0 : 1 +}; + +const getClosedPortUrl = async (): Promise => { + const server = createServer(); + await new Promise(resolve => { + server.listen(0, '127.0.0.1', resolve); + }); + + const {port} = server.address() as {port: number}; + await new Promise(resolve => { + server.close(resolve); + }); + + return `http://127.0.0.1:${port}`; +}; + const createSocketTimeoutStream = (): http.ClientRequest => { const stream = new PassThroughStream(); // @ts-expect-error Mocking the behaviour of a ClientRequest @@ -203,6 +249,139 @@ test('custom error codes', async t => { t.is(error.code, errorCode); }); +test('retries when ClientRequest emits a connection error before its end callback receives it', async t => { + let attemptCount = 0; + let beforeRetryCount = 0; + let beforeErrorCount = 0; + + const error = await t.throwsAsync(got('http://localhost', { + request: () => { + attemptCount++; + return createRequestWithEndError('request-error-first'); + }, + retry: { + limit: 2, + ...retryImmediately + }, + hooks: { + beforeRetry: [ + (_options, error) => { + beforeRetryCount++; + t.is(error?.code, 'ECONNREFUSED'); + } + ], + beforeError: [ + error => { + beforeErrorCount++; + return error; + } + ] + } + }), { + instanceOf: RequestError + }); + + t.is(attemptCount, 3); + t.is(beforeRetryCount, 2); + t.is(beforeErrorCount, 1); + t.is(error.code, 'ECONNREFUSED'); + t.is(error.request?.retryCount, 2); +}); + +test('retries when only the end callback receives the connection error', async t => { + let attemptCount = 0; + + const error = await t.throwsAsync(got('http://localhost', { + request: () => { + attemptCount++; + return createRequestWithEndError('end-callback-only'); + }, + retry: { + limit: 1, + ...retryImmediately + } + }), { + instanceOf: RequestError + }); + + t.is(attemptCount, 2); + t.is(error.code, 'ECONNREFUSED'); + t.is(error.request?.retryCount, 1); +}); + +test('recovers when retrying after a request end error', withServer, async (t, server, got) => { + server.get('/', (_request, response) => { + response.end('ok'); + }); + + let attemptCount = 0; + const response = await got({ + request: (url, options) => { + attemptCount++; + + if (attemptCount === 1) { + return createRequestWithEndError('end-callback-only'); + } + + return http.request(url, options); + }, + retry: { + limit: 1, + ...retryImmediately + } + }); + + t.is(response.body, 'ok'); + t.is(response.retryCount, 1); + t.is(attemptCount, 2); +}); + +test('end callback errors do not finish the stream before retrying', async t => { + const stream = got.stream('http://localhost', { + request: () => createRequestWithEndError('end-callback-only'), + retry: { + limit: 1, + ...retryImmediately + } + }); + + let finishCount = 0; + stream.on('finish', () => { + finishCount++; + }); + + await pEvent(stream, 'retry'); + + t.is(finishCount, 0); + t.false(stream.writableFinished); + stream.destroy(); +}); + +test('retries a refused connection and rejects with the connection error', async t => { + const url = await getClosedPortUrl(); + let beforeRetryCount = 0; + + const error = await t.throwsAsync(got(url, { + retry: { + limit: 2, + ...retryImmediately + }, + hooks: { + beforeRetry: [ + () => { + beforeRetryCount++; + } + ] + } + }), { + instanceOf: RequestError + }); + + t.is(error.code, 'ECONNREFUSED'); + t.is(beforeRetryCount, 2); + t.is(error.request?.retryCount, 2); +}); + test('respects 413 Retry-After', withServer, async (t, server, got) => { let lastTried413access = Date.now(); server.get('/', (_request, response) => {