Fix the retry functionality and increase coverage

source/errors.ts

@@ -1,5 +1,4 @@
 import urlLib from 'url';
-import http from 'http';
 import is from '@sindresorhus/is';
 import {Timings} from '@szmarczak/http-timer';
 import {Response, Options} from './utils/types';
@@ -65,14 +64,7 @@ export class HTTPError extends GotError {
 	response: Response;
 
 	constructor(response: Response, options: Options) {
-		const {statusCode} = response;
-		let {statusMessage} = response;
-
-		if (statusMessage) {
-			statusMessage = statusMessage.replace(/\r?\n/g, ' ').trim();
-		} else {
-			statusMessage = http.STATUS_CODES[statusCode];
-		}
+		const {statusCode, statusMessage} = response;
 
 		super(`Response code ${statusCode} (${statusMessage})`, {}, options);
 		this.name = 'HTTPError';

source/normalize-arguments.ts

@@ -220,10 +220,10 @@ export const normalizeArguments = (url, options, defaults?: any) => {
 				return 0;
 			}
 
-			const hasCode = Reflect.has(error, 'code') && options.retry.errorCodes.has(error.code);
-			const hasMethod = Reflect.has(error, 'options') && options.retry.methods.has(error.options.method);
+			const hasMethod = options.retry.methods.has(error.options.method);
+			const hasErrorCode = Reflect.has(error, 'code') && options.retry.errorCodes.has(error.code);
 			const hasStatusCode = Reflect.has(error, 'response') && options.retry.statusCodes.has(error.response.statusCode);
-			if ((!error || !hasCode) && (!hasMethod || !hasStatusCode)) {
+			if (!hasMethod || (!hasErrorCode && !hasStatusCode)) {
 				return 0;
 			}
 

source/request-as-event-emitter.ts

@@ -110,8 +110,8 @@ export default (options, input?: TransformStream) => {
 					});
 				}
 
-				const {statusCode} = response;
-				response.statusMessage = response.statusMessage || http.STATUS_CODES[statusCode];
+				const {statusCode, statusMessage} = response;
+				response.statusMessage = statusMessage ? statusMessage : http.STATUS_CODES[statusCode];
 				response.url = currentUrl;
 				response.requestUrl = requestUrl;
 				response.retryCount = retryCount;

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,7 @@
 import net from 'net';
-import {ClientRequest} from 'http';
+import {ClientRequest, IncomingMessage} from 'http';
 import {Delays} from './types';
+import unhandler from './unhandle';
 
 export class TimeoutError extends Error {
 	event: string;
@@ -20,22 +21,15 @@ const reentry: symbol = Symbol('reentry');
 const noop = (): void => {};
 
 export default (request: ClientRequest, delays: Delays, options: any) => {
-	/* istanbul ignore next: this makes sure timed-out isn't called twice */
 	if (Reflect.has(request, reentry)) {
 		return noop;
 	}
 
 	(request as any)[reentry] = true;
+	const cancelers: Array<typeof noop> = [];
+	const {once, unhandleAll} = unhandler();
 
-	let stopNewTimeouts = false;
-
-	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: any) => void, ...args: any): (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,19 +63,19 @@ export default (request: ClientRequest, delays: Delays, options: any) => {
 		request.abort();
 	};
 
-	const cancelers: Array<() => void> = [];
 	const cancelTimeouts = (): void => {
-		stopNewTimeouts = true;
 		cancelers.forEach(cancelTimeout => cancelTimeout());
+		unhandleAll();
 	};
 
 	request.on('error', (error: Error): void => {
 		if (error.message !== 'socket hang up') {
 			cancelTimeouts();
 		}
 	});
-	request.once('response', response => {
-		response.once('end', cancelTimeouts);
+
+	once(request, 'response', (response: IncomingMessage) => {
+		once(response, 'end', cancelTimeouts);
 	});
 
 	if (delays.request !== undefined) {
@@ -103,34 +97,34 @@ export default (request: ClientRequest, delays: Delays, options: any) => {
 		});
 	}
 
-	request.once('socket', (socket: net.Socket): void => {
+	once(request, 'socket', (socket: net.Socket): void => {
 		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 = () => 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): void => {
+					once(socket, 'lookup', (error: Error): void => {
 						if (error === null) {
-							socket.once('connect', timeConnect());
+							once(socket, 'connect', timeConnect());
 						}
 					});
 				}
 			}
 
 			if (delays.secureConnect !== undefined && options.protocol === 'https:') {
-				socket.once('connect', (): void => {
+				once(socket, 'connect', (): void => {
 					const cancelTimeout = addTimeout(delays.secureConnect!, timeoutHandler, 'secureConnect');
-					socket.once('secureConnect', cancelTimeout);
+					once(socket, 'secureConnect', cancelTimeout);
 				});
 			}
 		}
@@ -139,19 +133,19 @@ export default (request: ClientRequest, delays: Delays, options: any) => {
 			const timeRequest = () => addTimeout(delays.send!, timeoutHandler, 'send');
 			/* istanbul ignore next: hard to test */
 			if (socket.connecting) {
-				socket.once('connect', (): void => {
-					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', (): void => {
+		once(request, 'upload-complete', (): void => {
 			const cancelTimeout = addTimeout(delays.response!, timeoutHandler, 'response');
-			request.once('response', cancelTimeout);
+			once(request, 'response', cancelTimeout);
 		});
 	}
 

source/utils/unhandle.ts

@@ -0,0 +1,34 @@
+import EventEmitter from 'events';
+
+type Origin = EventEmitter;
+type Event = string | symbol;
+type Fn = (...args: any[]) => void;
+
+interface Handler {
+	origin: Origin;
+	event: Event;
+	fn: Fn;
+}
+
+interface Unhandler {
+	once: (origin: Origin, event: Event, fn: Fn) => void;
+	unhandleAll: () => void;
+}
+
+export default (): Unhandler => {
+	const handlers: Handler[] = [];
+
+	return {
+		once(origin: Origin, event: Event, fn: Fn) {
+			origin.once(event, fn);
+			handlers.push({origin, event, fn});
+		},
+
+		unhandleAll() {
+			handlers.forEach((handler: Handler) => {
+				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 from 'events';
 import http from 'http';
 import net from 'net';
 import getStream from 'get-stream';
 import test from 'ava';
 import pEvent from 'p-event';
 import delay from '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('assure 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);
+});