From 0501e00bff5430fd4cddcb6f59180744394aff9f Mon Sep 17 00:00:00 2001 From: Szymon Marczak <36894700+szmarczak@users.noreply.github.com> Date: Fri, 21 Jun 2019 15:55:45 +0200 Subject: [PATCH] Fix the retry functionality and increase coverage (#787) --- source/errors.ts | 10 +---- source/normalize-arguments.ts | 6 +-- source/utils/dynamic-require.ts | 2 + source/utils/timed-out.ts | 68 ++++++++++++++++----------------- source/utils/unhandle.ts | 38 ++++++++++++++++++ test/retry.ts | 15 ++++++++ test/timeout.ts | 33 ++++++++++++++++ 7 files changed, 126 insertions(+), 46 deletions(-) create mode 100644 source/utils/unhandle.ts diff --git a/source/errors.ts b/source/errors.ts index ced43608a..95d820723 100644 --- a/source/errors.ts +++ b/source/errors.ts @@ -1,5 +1,4 @@ import {format} from 'url'; -import {STATUS_CODES} from 'http'; import is from '@sindresorhus/is'; import {Timings} from '@szmarczak/http-timer'; import {Response, NormalizedOptions} from './utils/types'; @@ -63,14 +62,7 @@ export class HTTPError extends GotError { readonly response!: Response; constructor(response: Response, options: NormalizedOptions) { - const {statusCode} = response; - let {statusMessage} = response; - - if (statusMessage) { - statusMessage = statusMessage.replace(/\r?\n/g, ' ').trim(); - } else { - statusMessage = STATUS_CODES[statusCode]; - } + const {statusCode, statusMessage} = response; super(`Response code ${statusCode} (${statusMessage})`, {}, options); this.name = 'HTTPError'; diff --git a/source/normalize-arguments.ts b/source/normalize-arguments.ts index 9a0b9697e..bec64f170 100644 --- a/source/normalize-arguments.ts +++ b/source/normalize-arguments.ts @@ -240,10 +240,10 @@ export const normalizeArguments = (url: URLOrOptions, options: NormalizedOptions return 0; } - const hasCode = Reflect.has(error, 'code') && options.retry.errorCodes.has((error as GotError).code as ErrorCode); - const hasMethod = Reflect.has(error, 'options') && options.retry.methods.has((error as GotError).options.method as Method); + const hasMethod = options.retry.methods.has((error as GotError).options.method as Method); + const hasErrorCode = Reflect.has(error, 'code') && options.retry.errorCodes.has((error as GotError).code as ErrorCode); const hasStatusCode = Reflect.has(error, 'response') && options.retry.statusCodes.has((error as HTTPError | ParseError | MaxRedirectsError).response.statusCode as StatusCode); - if ((!error || !hasCode) && (!hasMethod || !hasStatusCode)) { + if (!hasMethod || (!hasErrorCode && !hasStatusCode)) { return 0; } diff --git a/source/utils/dynamic-require.ts b/source/utils/dynamic-require.ts index 505c00e43..11887dfc8 100644 --- a/source/utils/dynamic-require.ts +++ b/source/utils/dynamic-require.ts @@ -1,3 +1,5 @@ +/* istanbul ignore file: used for webpack */ + export default (moduleObject: NodeModule, moduleId: string): unknown => { return moduleObject.require(moduleId); }; diff --git a/source/utils/timed-out.ts b/source/utils/timed-out.ts index 8850ef001..620f07ede 100644 --- a/source/utils/timed-out.ts +++ b/source/utils/timed-out.ts @@ -1,6 +1,16 @@ import net = require('net'); -import {ClientRequest} from 'http'; -import {Delays, NormalizedOptions} from './types'; +import {ClientRequest, IncomingMessage} from 'http'; +import {Delays} from './types'; +import unhandler from './unhandle'; + +const reentry = Symbol('reentry'); +const noop = (): void => {}; + +interface TimedOutOptions { + host?: string; + hostname?: string; + protocol?: string; +} export class TimeoutError extends Error { code: string; @@ -13,27 +23,16 @@ export class TimeoutError extends Error { } } -const reentry = Symbol('reentry'); -const noop = (): void => {}; - -export default (request: ClientRequest, delays: Required, options: NormalizedOptions) => { - /* istanbul ignore next: this makes sure timed-out isn't called twice */ +export default (request: ClientRequest, delays: Delays, options: TimedOutOptions) => { if (Reflect.has(request, reentry)) { return noop; } request[reentry] = true; + const cancelers: Array = []; + const {once, unhandleAll} = unhandler(); - let stopNewTimeouts = false; - const cancelers: Array<() => void> = []; - - const addTimeout = (delay: number, callback: (...args: any[]) => void, ...args: any[]): (() => void) => { - // An error had been thrown before. Going further would result in uncaught errors. - // See https://github.com/sindresorhus/got/issues/631#issuecomment-435675051 - if (stopNewTimeouts) { - return noop; - } - + const addTimeout = (delay: number, callback: (...args: unknown[]) => void, ...args: unknown[]): (typeof noop) => { // Event loop order is timers, poll, immediates. // The timed event may emit during the current tick poll phase, so // defer calling the handler until the poll phase completes. @@ -69,10 +68,11 @@ export default (request: ClientRequest, delays: Required, options: Norma }; const cancelTimeouts = (): void => { - stopNewTimeouts = true; for (const cancel of cancelers) { cancel(); } + + unhandleAll(); }; request.on('error', error => { @@ -81,8 +81,8 @@ export default (request: ClientRequest, delays: Required, options: Norma } }); - request.once('response', response => { - response.once('end', cancelTimeouts); + once(request, 'response', (response: IncomingMessage): void => { + once(response, 'end', cancelTimeouts); }); if (delays.request !== undefined) { @@ -104,7 +104,7 @@ export default (request: ClientRequest, delays: Required, options: Norma }); } - request.once('socket', (socket: net.Socket) => { + once(request, 'socket', (socket: net.Socket): void => { // TODO: There seems to not be a 'socketPath' on the request, but there IS a socket.remoteAddress const {socketPath} = request as any; @@ -112,27 +112,27 @@ export default (request: ClientRequest, delays: Required, options: Norma if (socket.connecting) { if (delays.lookup !== undefined && !socketPath && !net.isIP(hostname || host)) { const cancelTimeout = addTimeout(delays.lookup, timeoutHandler, 'lookup'); - socket.once('lookup', cancelTimeout); + once(socket, 'lookup', cancelTimeout); } if (delays.connect !== undefined) { const timeConnect = (): (() => void) => addTimeout(delays.connect, timeoutHandler, 'connect'); if (socketPath || net.isIP(hostname || host)) { - socket.once('connect', timeConnect()); + once(socket, 'connect', timeConnect()); } else { - socket.once('lookup', (error: Error) => { - if (!error) { - socket.once('connect', timeConnect()); + once(socket, 'lookup', (error: Error): void => { + if (error === null) { + once(socket, 'connect', timeConnect()); } }); } } if (delays.secureConnect !== undefined && options.protocol === 'https:') { - socket.once('connect', () => { + once(socket, 'connect', (): void => { const cancelTimeout = addTimeout(delays.secureConnect, timeoutHandler, 'secureConnect'); - socket.once('secureConnect', cancelTimeout); + once(socket, 'secureConnect', cancelTimeout); }); } } @@ -141,19 +141,19 @@ export default (request: ClientRequest, delays: Required, options: Norma const timeRequest = (): (() => void) => addTimeout(delays.send, timeoutHandler, 'send'); /* istanbul ignore next: hard to test */ if (socket.connecting) { - socket.once('connect', () => { - request.once('upload-complete', timeRequest()); + once(socket, 'connect', (): void => { + once(request, 'upload-complete', timeRequest()); }); } else { - request.once('upload-complete', timeRequest()); + once(request, 'upload-complete', timeRequest()); } } }); if (delays.response !== undefined) { - request.once('upload-complete', () => { - const cancelTimeout = addTimeout(delays.response, timeoutHandler, 'response'); - request.once('response', cancelTimeout); + once(request, 'upload-complete', (): void => { + const cancelTimeout = addTimeout(delays.response!, timeoutHandler, 'response'); + once(request, 'response', cancelTimeout); }); } diff --git a/source/utils/unhandle.ts b/source/utils/unhandle.ts new file mode 100644 index 000000000..55d3d7f72 --- /dev/null +++ b/source/utils/unhandle.ts @@ -0,0 +1,38 @@ +import EventEmitter = require('events'); + +type Origin = EventEmitter; +type Event = string | symbol; +type Fn = (...args: unknown[]) => void; + +interface Handler { + origin: Origin; + event: Event; + fn: Fn; +} + +interface Unhandler { + once: (origin: Origin, event: Event, fn: Fn) => void; + unhandleAll: () => void; +} + +// When attaching listeners, it's very easy to forget about them. +// Especially if you do error handling and set timeouts. +// So instead of checking if it's proper to throw an error on every timeout ever, +// use this simple tool which will remove all listeners you have attached. +export default (): Unhandler => { + const handlers: Handler[] = []; + + return { + once(origin: Origin, event: Event, fn: Fn) { + origin.once(event, fn); + handlers.push({origin, event, fn}); + }, + + unhandleAll() { + for (const handler of handlers) { + const {origin, event, fn} = handler; + origin.removeListener(event, fn); + } + } + }; +}; diff --git a/test/retry.ts b/test/retry.ts index e89a60fb0..b45d41ffc 100644 --- a/test/retry.ts +++ b/test/retry.ts @@ -299,3 +299,18 @@ test('retry function can throw', withServer, async (t, server, got) => { } }), error); }); + +test('does not retry on POST', withServer, async (t, server, got) => { + server.post('/', () => {}); + + await t.throwsAsync(got.post({ + timeout: 200, + hooks: { + beforeRetry: [ + () => { + t.fail('Retries on POST requests'); + } + ] + } + }), got.TimeoutError); +}); diff --git a/test/timeout.ts b/test/timeout.ts index 8e7d5c974..e5efabbe0 100644 --- a/test/timeout.ts +++ b/test/timeout.ts @@ -1,3 +1,4 @@ +import EventEmitter = require('events'); import http = require('http'); import net = require('net'); import getStream = require('get-stream'); @@ -5,6 +6,7 @@ import test from 'ava'; import pEvent = require('p-event'); import delay = require('delay'); import got from '../source'; +import timedOut from '../source/utils/timed-out'; import withServer from './helpers/with-server'; import slowDataStream from './helpers/slow-data-stream'; @@ -432,3 +434,34 @@ test('no memory leak when using socket timeout and keepalive agent', withServer, t.is(socket.listenerCount('timeout'), 0); }); + +test('ensure there are no new timeouts after cancelation', t => { + const emitter = new EventEmitter(); + const socket = new EventEmitter(); + (socket as any).connecting = true; + + timedOut(emitter as http.ClientRequest, { + connect: 1 + }, { + hostname: '127.0.0.1' + })(); + + emitter.emit('socket', socket); + socket.emit('lookup', null); + t.is(socket.listenerCount('connect'), 0); +}); + +test('double calling timedOut has no effect', t => { + const emitter = new EventEmitter(); + + const attach = () => timedOut(emitter as http.ClientRequest, { + connect: 1 + }, { + hostname: '127.0.0.1' + }); + + attach(); + attach(); + + t.is(emitter.listenerCount('socket'), 1); +});