Repository: sindresorhus/got
Pull request #469 (2018-03-13 16:55:11)

Clear the progress interval if the socket has been destroyed
At work recently we ran into a problem where we used `got` in a script and the `progressInterval` is preventing node from being able to shut down.

If you make a request to a local server that is not running, [`req.once('error', ...`](https://github.com/sindresorhus/got/blob/7d1aa01e69229ad50ce355589681ff1d1a765627/index.js#L168) fires before [`req.once('request', ...`](https://github.com/sindresorhus/got/blob/7d1aa01e69229ad50ce355589681ff1d1a765627/index.js#L185). This causes the `progressInterval` to be cleared before it is set. Then `setInterval` gets called and your process will never exit. This is reliable using both `127.0.0.1` and my computer's local IP `192.168.1.165`. This does not trigger if you use the hostname `localhost`.

Because the result of this is that node just never exits, I don't know how to add this to your existing tests. I have this example that I just put in the root of the project to verify the bug and fix.

```js
// example.js
const got = require('./index.js')

got('http://127.0.0.1:55555', { retries: 0 })
.then(function (response) {
  console.log('response', response)
})
.catch(function (error) {
  console.log('error', error)
})
```

With the following local modifications for debugging

<details>
<summary>diff</summary>

```diff
diff --git a/index.js b/index.js
index 3cd9d09..ea2bc6a 100644
--- a/index.js
+++ b/index.js
@@ -166,6 +166,7 @@ function requestAsEventEmitter(opts) {
                        });
 
                        req.once('error', err => {
+                               console.log(`clearInterval(${progressInterval})`)
                                clearInterval(progressInterval);
 
                                if (aborted) {
@@ -197,6 +198,7 @@ function requestAsEventEmitter(opts) {
                                        const onSocketConnect = () => {
                                                const uploadEventFrequency = 150;
 
+                                               console.log('onSocketConnect')
                                                progressInterval = setInterval(() => {
                                                        if (socket.destroyed) {
                                                                clearInterval(progressInterval)

```

</details>

I get this output and the process hangs forever.

```
asa:~/repos/got$ node example.js 
clearInterval(undefined)
error { Error: connect ECONNREFUSED 127.0.0.1:55555
    at ClientRequest.req.once.err (/home/asa/repos/got/index.js:183:22)
    at Object.onceWrapper (events.js:316:30)
    at emitOne (events.js:115:13)
    at ClientRequest.emit (events.js:210:7)
    at Socket.socketErrorListener (_http_client.js:401:9)
    at emitOne (events.js:115:13)
    at Socket.emit (events.js:210:7)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
  name: 'RequestError',
  code: 'ECONNREFUSED',
  host: '127.0.0.1:55555',
  hostname: '127.0.0.1',
  method: 'GET',
  path: '/',
  protocol: 'http:',
  url: 'http://127.0.0.1:55555/' }
onSocketConnect
```


brandon93s: You may be able to test this with `process._getActiveHandles` with an assertion that the count is the same before and after the tested code. A failing test could be written demonstrating the lingering handle. 
AsaAyers: Thanks. It doesn't seem like a simple count works, I think ti's because ava runs tests in parallel. 

> Note that this only applies to tests within a particular test file. AVA will still run multiple tests files at the same time unless you pass the --serial CLI flag.

```
test.serial('Clear the progressInterval if the socket has been destroyed', async t => {
	const handleCount = process._getActiveHandles().length;
	const err = await t.throws(got(`http://127.0.0.1:55555/`, { retry: 0 }));
	t.is(err.code, 'ECONNREFUSED');
	t.is(process._getActiveHandles().length, handleCount)
})
```

I think it might work to see how many of the handles are timers. Using logging `Object.getPrototypeOf(handle)` I can see something that says its a `Timer`, but I don't know how else I can get a reference to that prototype to filter the list with. `Object.getPrototypeOf(setInterval(...` seems to give me a different thing named `Timer`.
szmarczak: @AsaAyers Hmm, I've checked your issue on Windows 10 (latest got version), I got such error, but there's no `onSocketConnect`. The process exits normally. Take a look:
```
>node blah
clearInterval(undefined)
error { RequestError: connect ECONNREFUSED 127.0.0.1:55555
    at ClientRequest.req.once.err (C:\Users\Szymon Marczak\Desktop\project\node_modules\got\index.js:183:22)
    at Object.onceWrapper (events.js:254:19)
    at ClientRequest.emit (events.js:159:13)
    at Socket.socketErrorListener (_http_client.js:389:9)
    at Socket.emit (events.js:159:13)
    at emitErrorNT (internal/streams/destroy.js:64:8)
    at _combinedTickCallback (internal/process/next_tick.js:138:11)
    at process._tickCallback (internal/process/next_tick.js:180:9)
  name: 'RequestError',
  code: 'ECONNREFUSED',
  host: '127.0.0.1:55555',
  hostname: '127.0.0.1',
  method: 'GET',
  path: '/',
  protocol: 'http:',
  url: 'http://127.0.0.1:55555/' }
```
AsaAyers: The problem exists on Mac and Linux, I didn't know if it would trigger on windows. 

Patch:
diff --git a/index.js b/index.js
index 2ca8862..db020d7 100644
--- a/index.js
+++ b/index.js
@@ -198,6 +198,11 @@ function requestAsEventEmitter(opts) {
 						const uploadEventFrequency = 150;
 
 						progressInterval = setInterval(() => {
+							if (socket.destroyed) {
+								clearInterval(progressInterval);
+								return;
+							}
+
 							const lastUploaded = uploaded;
 							const headersSize = Buffer.byteLength(req._header);
 							uploaded = socket.bytesWritten - headersSize;
@@ -527,8 +532,8 @@ function normalizeArguments(url, opts) {
 		'user-agent': `${pkg.name}/${pkg.version} (https://github.com/sindresorhus/got)`
 	}, headers);
 
-	if (opts.decompress) {
-		opts.headers['accept-encoding'] = 'gzip,deflate';
+	if (opts.decompress && is.undefined(opts.headers['accept-encoding'])) {
+		opts.headers['accept-encoding'] = 'gzip, deflate';
 	}
 
 	const query = opts.query;
diff --git a/readme.md b/readme.md
index 1691276..2347077 100644
--- a/readme.md
+++ b/readme.md
@@ -1,7 +1,7 @@
 <div align="center">
 	<br>
 	<br>
-	<img width="360" src="https://rawgit.com/sindresorhus/got/master/media/logo.svg" alt="got">
+	<img width="360" src="media/logo.svg" alt="Got">
 	<br>
 	<br>
 	<br>
@@ -195,7 +195,7 @@ request the resource pointed to in the location header via `GET`. This is in acc
 Type: `boolean`<br>
 Default: `true`
 
-Decompress the response automatically.
+Decompress the response automatically. This will set the `accept-encoding` header to `gzip, deflate` unless you set it yourself.
 
 If this is disabled, a compressed response is returned as a `Buffer`. This may be useful if you want to handle decompression yourself or stream the raw compressed data.
 


Test patch:
diff --git a/test/headers.js b/test/headers.js
index ba07cf5..ed7c056 100644
--- a/test/headers.js
+++ b/test/headers.js
@@ -24,7 +24,17 @@ test('user-agent', async t => {
 
 test('accept-encoding', async t => {
 	const headers = (await got(s.url, {json: true})).body;
-	t.is(headers['accept-encoding'], 'gzip,deflate');
+	t.is(headers['accept-encoding'], 'gzip, deflate');
+});
+
+test('do not override accept-encoding', async t => {
+	const headers = (await got(s.url, {
+		json: true,
+		headers: {
+			'accept-encoding': 'gzip'
+		}
+	})).body;
+	t.is(headers['accept-encoding'], 'gzip');
 });
 
 test('do not set accept-encoding header when decompress options is false', async t => {
diff --git a/test/socket-destroyed.js b/test/socket-destroyed.js
new file mode 100644
index 0000000..ce65795
--- /dev/null
+++ b/test/socket-destroyed.js
@@ -0,0 +1,9 @@
+import test from 'ava';
+import got from '..';
+
+test.serial('clear the progressInterval if the socket has been destroyed', async t => {
+	const handlesComingFromAVA = 2;
+	const err = await t.throws(got(`http://127.0.0.1:55555`, {retry: 0}));
+	t.is(process._getActiveHandles().length - handlesComingFromAVA, 2);
+	t.is(err.code, 'ECONNREFUSED');
+});