Repository: sindresorhus/got
Pull request #459 (2018-02-23 12:14:07)

Prevent uncaught exception in some cases
If something goes wrong in the code contained by setImmediate, an uncaughtException is thrown.

The code in this pull requests wraps the code in a try/catch block and emits an error if an error is thrown.

kevva: Could you add a test for this?
wwwouter: I've put the code in an `getResponse` function and added a test.
This test will fail with the old code, but succeeds with the changes.

I've also added some files to work on VS Code on Windows. I can remove these if you want.
wwwouter: Also: Travis is timing out, but this shows it's ok for at least node 4 and 8: https://travis-ci.org/sindresorhus/got/builds/346221646 
kevva: > I've also added some files to work on VS Code on Windows. I can remove these if you want.

Yes :).

Patch:
diff --git a/index.js b/index.js
index 0d9eb0b..6e7122c 100644
--- a/index.js
+++ b/index.js
@@ -142,59 +142,12 @@ function requestAsEventEmitter(opts) {
 				return;
 			}
 
-			const downloadBodySize = Number(res.headers['content-length']) || null;
-			let downloaded = 0;
-
 			setImmediate(() => {
-				const progressStream = new Transform({
-					transform(chunk, encoding, callback) {
-						downloaded += chunk.length;
-
-						const percent = downloadBodySize ? downloaded / downloadBodySize : 0;
-
-						// Let flush() be responsible for emitting the last event
-						if (percent < 1) {
-							ee.emit('downloadProgress', {
-								percent,
-								transferred: downloaded,
-								total: downloadBodySize
-							});
-						}
-
-						callback(null, chunk);
-					},
-
-					flush(callback) {
-						ee.emit('downloadProgress', {
-							percent: 1,
-							transferred: downloaded,
-							total: downloadBodySize
-						});
-
-						callback();
-					}
-				});
-
-				mimicResponse(res, progressStream);
-				progressStream.redirectUrls = redirects;
-
-				const response = opts.decompress === true &&
-					is.function(decompressResponse) &&
-					opts.method !== 'HEAD' ? decompressResponse(progressStream) : progressStream;
-
-				if (!opts.decompress && ['gzip', 'deflate'].indexOf(res.headers['content-encoding']) !== -1) {
-					opts.encoding = null;
+				try {
+					getResponse(res, opts, ee, redirects);
+				} catch (e) {
+					ee.emit('error', e);
 				}
-
-				ee.emit('response', response);
-
-				ee.emit('downloadProgress', {
-					percent: 0,
-					transferred: 0,
-					total: downloadBodySize
-				});
-
-				res.pipe(progressStream);
 			});
 		});
 
@@ -227,14 +180,18 @@ function requestAsEventEmitter(opts) {
 					total: uploadBodySize
 				});
 
-				if (req.connection) {
-					req.connection.once('connect', () => {
+				const socket = req.connection;
+				if (socket) {
+					// `._connecting` was the old property which was made public in node v6.1.0
+					const isConnecting = socket.connecting === undefined ? socket._connecting : socket.connecting;
+
+					const onSocketConnect = () => {
 						const uploadEventFrequency = 150;
 
 						progressInterval = setInterval(() => {
 							const lastUploaded = uploaded;
 							const headersSize = Buffer.byteLength(req._header);
-							uploaded = req.connection.bytesWritten - headersSize;
+							uploaded = socket.bytesWritten - headersSize;
 
 							// Prevent the known issue of `bytesWritten` being larger than body size
 							if (uploadBodySize && uploaded > uploadBodySize) {
@@ -254,7 +211,17 @@ function requestAsEventEmitter(opts) {
 								total: uploadBodySize
 							});
 						}, uploadEventFrequency);
-					});
+					};
+
+					// Only subscribe to 'connect' event if we're actually connecting a new
+					// socket, otherwise if we're already connected (because this is a
+					// keep-alive connection) do not bother. This is important since we won't
+					// get a 'connect' event for an already connected socket.
+					if (isConnecting) {
+						socket.once('connect', onSocketConnect);
+					} else {
+						onSocketConnect();
+					}
 				}
 			});
 
@@ -283,6 +250,61 @@ function requestAsEventEmitter(opts) {
 	return ee;
 }
 
+function getResponse(res, opts, ee, redirects) {
+	const downloadBodySize = Number(res.headers['content-length']) || null;
+	let downloaded = 0;
+
+	const progressStream = new Transform({
+		transform(chunk, encoding, callback) {
+			downloaded += chunk.length;
+
+			const percent = downloadBodySize ? downloaded / downloadBodySize : 0;
+
+			// Let flush() be responsible for emitting the last event
+			if (percent < 1) {
+				ee.emit('downloadProgress', {
+					percent,
+					transferred: downloaded,
+					total: downloadBodySize
+				});
+			}
+
+			callback(null, chunk);
+		},
+
+		flush(callback) {
+			ee.emit('downloadProgress', {
+				percent: 1,
+				transferred: downloaded,
+				total: downloadBodySize
+			});
+
+			callback();
+		}
+	});
+
+	mimicResponse(res, progressStream);
+	progressStream.redirectUrls = redirects;
+
+	const response = opts.decompress === true &&
+		is.function(decompressResponse) &&
+		opts.method !== 'HEAD' ? decompressResponse(progressStream) : progressStream;
+
+	if (!opts.decompress && ['gzip', 'deflate'].indexOf(res.headers['content-encoding']) !== -1) {
+		opts.encoding = null;
+	}
+
+	ee.emit('response', response);
+
+	ee.emit('downloadProgress', {
+		percent: 0,
+		transferred: 0,
+		total: downloadBodySize
+	});
+
+	res.pipe(progressStream);
+}
+
 function asPromise(opts) {
 	const timeoutFn = requestPromise => opts.gotTimeout && opts.gotTimeout.request ?
 		pTimeout(requestPromise, opts.gotTimeout.request, new got.RequestError({message: 'Request timed out', code: 'ETIMEDOUT'}, opts)) :
diff --git a/package.json b/package.json
index c545fb2..e55fd7d 100644
--- a/package.json
+++ b/package.json
@@ -77,6 +77,7 @@
 		"nyc": "^11.0.2",
 		"p-event": "^1.3.0",
 		"pem": "^1.4.4",
+		"proxyquire": "^1.8.0",
 		"sinon": "^4.0.0",
 		"slow-stream": "0.0.4",
 		"tempfile": "^2.0.0",


Test patch:
diff --git a/test/agent.js b/test/agent.js
index 3ccdc70..062b671 100644
--- a/test/agent.js
+++ b/test/agent.js
@@ -134,10 +134,14 @@ test('redirects from https to http work with an agent object', async t => {
 
 test('socket connect listener cleaned up after request', async t => {
 	const {agent} = createAgentSpy(HttpsAgent);
-	await got(`${https.url}`, {
-		rejectUnauthorized: false,
-		agent
-	});
+	// Make sure there are no memory leaks when reusing keep-alive sockets
+	for (let i = 0; i < 20; i++) {
+		// eslint-disable-next-line no-await-in-loop
+		await got(`${https.url}`, {
+			rejectUnauthorized: false,
+			agent
+		});
+	}
 	Object.keys(agent.freeSockets).forEach(k => agent.freeSockets[k]
 		.forEach(sock => t.is(sock.listenerCount('connect'), 0)));
 	// Make sure to close all open sockets
diff --git a/test/error.js b/test/error.js
index 1fe3f1d..ba63c45 100644
--- a/test/error.js
+++ b/test/error.js
@@ -1,6 +1,7 @@
 import http from 'http';
 import test from 'ava';
 import sinon from 'sinon';
+import proxyquire from 'proxyquire';
 import got from '..';
 import {createServer} from './helpers/server';
 
@@ -52,7 +53,7 @@ test('dns message', async t => {
 });
 
 test('options.body error message', async t => {
-	const err = await t.throws(got(s.url, {body: () => {}}));
+	const err = await t.throws(got(s.url, {body: () => { }}));
 	t.regex(err.message, /The `body` option must be a stream\.Readable, string, Buffer or plain Object/);
 });
 
@@ -78,6 +79,17 @@ test.serial('http.request error', async t => {
 	stub.restore();
 });
 
+test.serial('catch error in mimicResponse', async t => {
+	const proxiedGot = proxyquire('..', {
+		'mimic-response'() {
+			throw new Error('Error in mimic-response');
+		}
+	});
+
+	const err = await t.throws(proxiedGot(s.url));
+	t.is(err.message, 'Error in mimic-response');
+});
+
 test.after('cleanup', async () => {
 	await s.close();
 });
diff --git a/test/unix-socket.js b/test/unix-socket.js
index ba32935..63b4b22 100644
--- a/test/unix-socket.js
+++ b/test/unix-socket.js
@@ -8,35 +8,37 @@ const socketPath = tempy.file({extension: 'socket'});
 
 let s;
 
-test.before('setup', async () => {
-	s = await createServer();
+if (process.platform !== 'win32') {
+	test.before('setup', async () => {
+		s = await createServer();
 
-	s.on('/', (req, res) => {
-		res.end('ok');
-	});
+		s.on('/', (req, res) => {
+			res.end('ok');
+		});
 
-	s.on('/foo:bar', (req, res) => {
-		res.end('ok');
-	});
+		s.on('/foo:bar', (req, res) => {
+			res.end('ok');
+		});
 
-	await s.listen(socketPath);
-});
+		await s.listen(socketPath);
+	});
 
-test('works', async t => {
-	const url = format('http://unix:%s:%s', socketPath, '/');
-	t.is((await got(url)).body, 'ok');
-});
+	test('works', async t => {
+		const url = format('http://unix:%s:%s', socketPath, '/');
+		t.is((await got(url)).body, 'ok');
+	});
 
-test('protocol-less works', async t => {
-	const url = format('unix:%s:%s', socketPath, '/');
-	t.is((await got(url)).body, 'ok');
-});
+	test('protocol-less works', async t => {
+		const url = format('unix:%s:%s', socketPath, '/');
+		t.is((await got(url)).body, 'ok');
+	});
 
-test('address with : works', async t => {
-	const url = format('unix:%s:%s', socketPath, '/foo:bar');
-	t.is((await got(url)).body, 'ok');
-});
+	test('address with : works', async t => {
+		const url = format('unix:%s:%s', socketPath, '/foo:bar');
+		t.is((await got(url)).body, 'ok');
+	});
 
-test.after('cleanup', async () => {
-	await s.close();
-});
+	test.after('cleanup', async () => {
+		await s.close();
+	});
+}