Fix the retry functionality and increase coverage (#787)

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

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

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);
 };

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<Delays>, 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<typeof noop> = [];
+	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<Delays>, 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<Delays>, 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,35 +104,35 @@ export default (request: ClientRequest, delays: Required<Delays>, 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;
 
 		/* istanbul ignore next: hard to test */
 		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<Delays>, 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);
 		});
 	}
 

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);
+			}
+		}
+	};
+};

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);
+});

test/timeout.ts

@@ -1,10 +1,12 @@
+import EventEmitter = require('events');
 import http = require('http');
 import net = require('net');
 import getStream = require('get-stream');
 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);
+});