Fix bugs, increase coverage, update benchmark results

Fixes #1187

benchmark/index.ts

@@ -183,18 +183,18 @@ const internalBenchmark = (): void => {
 };
 
 // Results (i7-7700k, CPU governor: performance):
-// got - promise                          x 3,092 ops/sec ±5.25% (73 runs sampled)
-// got - stream                           x 4,313 ops/sec ±5.61% (72 runs sampled)
-// got - promise core                     x 6,756 ops/sec ±5.32% (80 runs sampled)
-// got - stream core                      x 6,863 ops/sec ±4.68% (76 runs sampled)
-// got - stream core - normalized options x 7,960 ops/sec ±3.83% (81 runs sampled)
-// request - callback                     x 6,912 ops/sec ±6.50% (76 runs sampled)
-// request - stream                       x 7,821 ops/sec ±4.28% (80 runs sampled)
-// node-fetch - promise                   x 7,036 ops/sec ±8.17% (78 runs sampled)
-// node-fetch - stream                    x 7,877 ops/sec ±4.17% (80 runs sampled)
-// axios - promise                        x 6,613 ops/sec ±3.22% (76 runs sampled)
-// axios - stream                         x 8,642 ops/sec ±2.84% (79 runs sampled)
-// https - stream                         x 9,955 ops/sec ±6.36% (76 runs sampled)
+// got - promise                          x 3,204 ops/sec ±5.27% (73 runs sampled)
+// got - stream                           x 5,045 ops/sec ±3.85% (77 runs sampled)
+// got - promise core                     x 6,499 ops/sec ±3.67% (77 runs sampled)
+// got - stream core                      x 7,047 ops/sec ±2.32% (83 runs sampled)
+// got - stream core - normalized options x 7,313 ops/sec ±2.79% (85 runs sampled)
+// request - callback                     x 6,918 ops/sec ±5.76% (73 runs sampled)
+// request - stream                       x 7,746 ops/sec ±3.20% (82 runs sampled)
+// node-fetch - promise                   x 7,011 ops/sec ±7.54% (75 runs sampled)
+// node-fetch - stream                    x 7,941 ops/sec ±4.52% (82 runs sampled)
+// axios - promise                        x 6,788 ops/sec ±3.32% (80 runs sampled)
+// axios - stream                         x 8,584 ops/sec ±2.23% (81 runs sampled)
+// https - stream                         x 10,465 ops/sec ±2.89% (73 runs sampled)
 // Fastest is https - stream
 
 // got - normalize options x 166,389 ops/sec ±0.63% (91 runs sampled)

source/as-promise/core.ts

@@ -33,12 +33,10 @@ export const parseBody = (response: Response, responseType: ResponseType, encodi
 			return Buffer.from(rawBody);
 		}
 
-		if (!knownBodyTypes.includes(responseType)) {
-			throw new ParseError({
-				message: `Unknown body type '${responseType as string}'`,
-				name: 'Error'
-			}, response);
-		}
+		throw new ParseError({
+			message: `Unknown body type '${responseType as string}'`,
+			name: 'Error'
+		}, response);
 	} catch (error) {
 		throw new ParseError(error, response);
 	}

source/as-promise/index.ts

@@ -7,8 +7,7 @@ import {
 	CancelableRequest,
 	Response,
 	RequestError,
-	HTTPError,
-	ReadError
+	HTTPError
 } from './types';
 import PromisableRequest, {parseBody} from './core';
 import proxyEvents from '../core/utils/proxy-events';
@@ -80,9 +79,9 @@ export default function asPromise<T>(options: NormalizedOptions): CancelableRequ
 					rawBody = await getStream.buffer(request);
 
 					response.rawBody = rawBody;
-				} catch (error) {
-					// TODO: Call `request._beforeError`, see https://github.com/nodejs/node/issues/32995
-					reject(new ReadError(error, request));
+				} catch (_) {
+					// The same error is caught below.
+					// See request.once('error')
 					return;
 				}
 
@@ -135,12 +134,7 @@ export default function asPromise<T>(options: NormalizedOptions): CancelableRequ
 					}
 				} catch (error) {
 					// TODO: Call `request._beforeError`, see https://github.com/nodejs/node/issues/32995
-					if (error instanceof RequestError) {
-						reject(error);
-					} else {
-						reject(new RequestError(error.message, error, request));
-					}
-
+					reject(new RequestError(error.message, error, request));
 					return;
 				}
 

source/core/index.ts

@@ -469,7 +469,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 
 	declare options: NormalizedOptions;
 	declare requestUrl: string;
-	finalized: boolean;
+	requestInitialized: boolean;
 	redirects: string[];
 
 	constructor(url: string | URL, options: Options = {}, defaults?: Defaults) {
@@ -480,7 +480,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 
 		this[kDownloadedSize] = 0;
 		this[kUploadedSize] = 0;
-		this.finalized = false;
+		this.requestInitialized = false;
 		this[kServerResponsesPiped] = new Set<ServerResponse>();
 		this.redirects = [];
 		this[kStopReading] = false;
@@ -558,7 +558,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 					job();
 				}
 
-				this.finalized = true;
+				this.requestInitialized = true;
 			} catch (error) {
 				if (error instanceof RequestError) {
 					this._beforeError(error);
@@ -674,11 +674,9 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			options.searchParams = new URLSearchParams(options.searchParams as Record<string, string>);
 
 			// `normalizeArguments()` is also used to merge options
-			if (defaults?.searchParams) {
-				defaults.searchParams.forEach((value, key) => {
-					(options!.searchParams as URLSearchParams).append(key, value);
-				});
-			}
+			defaults?.searchParams?.forEach((value, key) => {
+				(options!.searchParams as URLSearchParams).append(key, value);
+			});
 		} else {
 			options.searchParams = defaults?.searchParams;
 		}
@@ -992,7 +990,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			this.emit('downloadProgress', this.downloadProgress);
 		});
 
-		response.on('error', (error: Error) => {
+		response.once('error', (error: Error) => {
 			this._beforeError(new ReadError(error, this));
 		});
 
@@ -1415,7 +1413,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			this._writeRequest(chunk, encoding, callback);
 		};
 
-		if (this.finalized) {
+		if (this.requestInitialized) {
 			write();
 		} else {
 			this[kJobs].push(write);
@@ -1470,7 +1468,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 			});
 		};
 
-		if (this.finalized) {
+		if (this.requestInitialized) {
 			endRequest();
 		} else {
 			this[kJobs].push(endRequest);
@@ -1479,6 +1477,8 @@ export default class Request extends Duplex implements RequestEvents<Request> {
 
 	_destroy(error: Error | null, callback: (error: Error | null) => void): void {
 		if (kRequest in this) {
+			this[kCancelTimeouts]!();
+
 			// TODO: Remove the next `if` when these get fixed:
 			// - https://github.com/nodejs/node/issues/32851
 			// - https://github.com/nock/nock/issues/1981

source/core/utils/timed-out.ts

@@ -83,6 +83,7 @@ export default (request: ClientRequest, delays: Delays, options: TimedOutOptions
 		cancelTimeouts();
 
 		// Save original behavior
+		/* istanbul ignore next */
 		if (request.listenerCount('error') === 0) {
 			throw error;
 		}

source/create.ts

@@ -73,10 +73,10 @@ const aliases: readonly HTTPAlias[] = [
 
 export const defaultHandler: HandlerFunction = (options, next) => next(options);
 
-const callInitHooks = (hooks: InitHook[] | undefined, options: Options): void => {
+const callInitHooks = (hooks: InitHook[] | undefined, options?: Options): void => {
 	if (hooks) {
 		for (const hook of hooks) {
-			hook(options);
+			hook(options!);
 		}
 	}
 };
@@ -110,7 +110,7 @@ const create = (defaults: InstanceDefaults): Got => {
 		return result;
 	}));
 
-	const got: Got = ((url: string | URL, options: Options = {}): GotReturn => {
+	const got: Got = ((url: string | URL, options?: Options): GotReturn => {
 		let iteration = 0;
 		const iterateHandlers = (newOptions: NormalizedOptions): GotReturn => {
 			return defaults.handlers[iteration++](

test/arguments.ts

@@ -477,6 +477,15 @@ test('normalizes search params included in input', t => {
 	t.is(url.search, '?a=b+c');
 });
 
+test('normalizes search params included in options', t => {
+	const {url} = got.mergeOptions({
+		url: new URL('https://example.com'),
+		searchParams: 'a=b c'
+	});
+
+	t.is(url.search, '?a=b+c');
+});
+
 test('reuse options while using init hook', withServer, async (t, server, got) => {
 	t.plan(2);
 

test/cancel.ts

@@ -2,6 +2,7 @@ import {EventEmitter} from 'events';
 import {Readable as ReadableStream} from 'stream';
 import stream = require('stream');
 import test from 'ava';
+import delay = require('delay');
 import pEvent = require('p-event');
 import getStream = require('get-stream');
 import {Handler} from 'express';
@@ -75,6 +76,31 @@ test.serial('does not retry after cancelation', withServerAndLolex, async (t, se
 	await t.notThrowsAsync(promise, 'Request finished instead of aborting.');
 });
 
+test.serial('cleans up request timeouts', withServer, async (t, server, got) => {
+	server.get('/', () => {});
+
+	const gotPromise = got('redirect', {
+		timeout: 10,
+		retry: {
+			calculateDelay: ({computedValue}) => {
+				process.nextTick(() => gotPromise.cancel());
+
+				if (computedValue) {
+					return 20;
+				}
+
+				return 0;
+			},
+			limit: 1
+		}
+	});
+
+	await t.throwsAsync(gotPromise, {instanceOf: CancelError});
+
+	// Wait for unhandled errors
+	await delay(40);
+});
+
 test.serial('cancels in-progress request', withServerAndLolex, async (t, server, got, clock) => {
 	const {emitter, promise} = prepareServer(server, clock);
 
@@ -219,3 +245,18 @@ test('throws when canceling cached request', withServer, async (t, server, got)
 
 	await t.throwsAsync(promise, {instanceOf: got.CancelError});
 });
+
+test('throws when canceling cached request #2', withServer, async (t, server, got) => {
+	server.get('/', (_request, response) => {
+		response.setHeader('Cache-Control', 'public, max-age=60');
+		response.end(Date.now().toString());
+	});
+
+	const cache = new Map();
+	await got({cache});
+
+	const promise = got({cache});
+	promise.cancel();
+
+	await t.throwsAsync(promise, {instanceOf: got.CancelError});
+});

test/stream.ts

@@ -379,11 +379,13 @@ test('the socket is alive on a successful pipeline', withServer, async (t, serve
 	});
 
 	const gotStream = got.stream('');
+	t.is(gotStream.socket, undefined);
 
 	const receiver = new stream.PassThrough();
 	await promisify(stream.pipeline)(gotStream, receiver);
 
 	t.is(await getStream(receiver), payload);
+	t.truthy(gotStream.socket);
 	t.false(gotStream.socket!.destroyed);
 });
 

test/timeout.ts

@@ -455,6 +455,31 @@ test.serial('no unhandled timeout errors', withServer, async (t, _server, got) =
 	await delay(200);
 });
 
+// TODO: use lolex here
+test.serial('no unhandled timeout errors #2', withServer, async (t, server, got) => {
+	server.get('/', (_request, response) => {
+		response.write('Hello world!');
+	});
+
+	const gotPromise = got('', {
+		timeout: 20,
+		retry: {
+			calculateDelay: ({computedValue}) => {
+				if (computedValue) {
+					return 10;
+				}
+
+				return 0;
+			},
+			limit: 1
+		}
+	});
+
+	await t.throwsAsync(gotPromise, {instanceOf: TimeoutError});
+
+	await delay(100);
+});
+
 test.serial('no more timeouts after an error', withServer, async (t, _server, got) => {
 	const {setTimeout} = global;
 	const {clearTimeout} = global;

test/weakable-map.ts

@@ -0,0 +1,22 @@
+import test from 'ava';
+import WeakableMap from '../source/core/utils/weakable-map';
+
+test('works as expected', t => {
+	const weakable = new WeakableMap();
+
+	weakable.set('hello', 'world');
+
+	t.true(weakable.has('hello'));
+	t.false(weakable.has('foobar'));
+	t.is(weakable.get('hello'), 'world');
+	t.is(weakable.get('foobar'), undefined);
+
+	const object = {};
+	const anotherObject = {};
+	weakable.set(object, 'world');
+
+	t.true(weakable.has(object));
+	t.false(weakable.has(anotherObject));
+	t.is(weakable.get(object), 'world');
+	t.is(weakable.get(anotherObject), undefined);
+});