Request hangs up with GOT v11

[adityapatadia]

Describe the bug

• Node.js version: 12.16.3
• OS & version: Debian Buster

Actual behavior

Request gets stuck while downloading.
...

Expected behavior

Request should either timeout or give correct response
...

Code to reproduce

const agent = {
  http: new http.Agent({ keepAlive: true }),
  https: new https.Agent({ keepAlive: true })
};

var requestOptions = {
    timeout: 45000, // milliseconds
    responseType: 'buffer',
    retry: 0,
    maxRedirects: 5,
    dnsCache: false,
    headers: {
      'User-Agent': 'Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:57.0) Gecko/20100101 Firefox/57.0',
      'Accept': 'text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8',
      'Accept-Language': 'en-US,en;q=0.5'
    },
    shared: false,
    agent,
    cache: cacheStore
  };

got.get("https://www.empiresuppliesonline.co.uk/ekmps/shops/empiresupplies/images/Waring-X-Prep-Kitchen-Blender-2523-p.jpg", requestOptions);

I am using https://www.npmjs.com/package/improved-cacheable-request as cache store.

Please note I download around 500 such URLs per minute but the issue majorly happens with this given domain name.

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

I cannot reproduce: https://runkit.com/szmarczak/5ed626c21e7b4f001a7c618c

[michael42]

I think this might the issue we're currently having in our project.

Minimal repo (Node v12.18.0) (edit: added ETag-Header on 304 response):

const got = require('got');
const http = require('http');
const util = require('util');

const cache = new Map();
http.createServer((req, res) => {
  if (req.headers['if-none-match'] === 'asdf') {
    res.writeHead(304, { 'ETag': 'asdf' });
    return res.end();
  }
  res.writeHead(200, { 'ETag': 'asdf' });
  res.end(Buffer.from('content', 'utf-8'));
})
  .listen(1234, async () => {
    try {
      console.log('Starting request 1...');
      const res1 = await got('http://localhost:1234', { cache });
      console.log('Finished request 1');
      console.dir(res1.body);

      console.log('Starting request 2...');
      const res2 = await got('http://localhost:1234', { cache });
      console.log('Finished request 2');
      console.dir(res2.body);

      console.log('Finished');
      process.exit(0);
    } catch (e) {
      console.error(e);
      process.exit(1);
    }
  });

[email protected] seems to work fine:

Starting request 1...
Finished request 1
'content'
Starting request 2...
Finished request 2
'content'
Finished

[email protected] returns an empty response on the second request:

Starting request 1...
Finished request 1
'content'
Starting request 2...
Finished request 2
''
Finished

[email protected] and [email protected] hangs forever:

Starting request 1...
Finished request 1
'content'
Starting request 2...

Any idea of what's wrong here or should I open a new issue?

[Giotino]

About the cache problem (raised by @michael42) I can say that, on the second request, the CacheableRequest response do not carry the original request (because it's a clone of the previous one), so the cacheableResponse event is never fired.

@szmarczak
Take a look at this workaround master...Giotino:issue-1287

[szmarczak]

so the cacheableResponse event is never fired.

Then the promise resolves with a Response and _onResponse gets called.

[szmarczak]

got/source/core/index.ts

Lines 1315 to 1319 in cb4da8d

	if (req) {
	req.emit('cacheableResponse', typedResponse);
	}
	resolve(typedResponse as unknown as ResponseLike);

got/source/core/index.ts

Lines 1464 to 1478 in cb4da8d

	if (isClientRequest(requestOrResponse)) {
	this._onRequest(requestOrResponse);
	// Emit the response after the stream has been ended
	} else if (this.writable) {
	this.once('finish', () => {
	this._onResponse(requestOrResponse as IncomingMessageWithTimings);
	});
	this._unlockWrite();
	this.end();
	this._lockWrite();
	} else {
	this._onResponse(requestOrResponse as IncomingMessageWithTimings);
	}

[szmarczak]

I mean it should, but I haven't checked. I've been busy for a while and will be for 2 more weeks.

[Giotino]

so the cacheableResponse event is never fired.

Then the promise resolves with a Response and _onResponse gets called.

With some "High quality debugging" (console.log) I can say that _onResponse is not called the second time.

Starting request 1...
_onResponse
Finished request 1
'content'
Starting request 2...

I'm going to look into why that doesn't happen.

[Giotino]

so the cacheableResponse event is never fired.

Then the promise resolves with a Response and _onResponse gets called.

That's actually not correct, because the promise is resolved by this event, that occur before the callback, with the request.

got/source/core/index.ts

Line 1326 in cb4da8d

cacheRequest.once('request', resolve);

[szmarczak]

Then for some reason it doesn't have req property defined...

[Giotino]

Then for some reason it doesn't have req property defined...

That's because cacheableResponse answer with a clone of the previous response (if it exists in the cache) and this clone does not have the new request as req.

You were using the req property that was undocumented (https://github.com/lukechilds/responselike).
While the request event is documented, so I think that my PR is the correct answer to this problem.

[szmarczak]

That's because cacheableResponse answer with a clone of the previous response (if it exists in the cache)

Then the promise should resolve with a Response. It shouldn't create a real request and return cached response. Oh, I see it now! It creates the request to verify the etag header. Good catch!

[dapuicon]

Similar Issue:
In version: 11.3.0 request hangs forever when sending "authorization" header.

const headersToSend={"authorization": "Bearer ..."};

const response = await got(this.url, { headers: headersToSend, timeout: 1000 });

In version: 10.7.0 works!!

[Giotino]

Similar Issue:
In version: 11.3.0 request hangs forever when sending "authorization" header.

const headersToSend={"authorization": "Bearer ..."};

const response = await got(this.url, { headers: headersToSend, timeout: 1000 });

In version: 10.7.0 works!!

This issue is unrelated to the previous, because the problem was due to the cache.

Anyways, it's still an issue, but I can't reproduce it.

Take a look at this code and tell me if it reflect your case.
The server is a dummy server that never answer to the requests.

import got from 'got';
import express = require('express');

const app = express();
app.use('/', (_req: any, _res: any, _next: any) => {});

(async () => {
  const server = app.listen(4444);

  await got('http://127.0.0.1:4444/', {
    headers: {"authorization": "Bearer SOMETHING"},
    timeout: 1000
  });

  console.log('END');

  server.close();
})();

[dapuicon]

Similar Issue:
In version: 11.3.0 request hangs forever when sending "authorization" header.

const headersToSend={"authorization": "Bearer ..."};

const response = await got(this.url, { headers: headersToSend, timeout: 1000 });

In version: 10.7.0 works!!

This issue is unrelated to the previous, because the problem was due to the cache.

Anyways, it's still an issue, but I can't reproduce it.

Take a look at this code and tell me if it reflect your case.
The server is a dummy server that never answer to the requests.

import got from 'got';
import express = require('express');

const app = express();
app.use('/', (_req: any, _res: any, _next: any) => {});

(async () => {
  const server = app.listen(4444);

  await got('http://127.0.0.1:4444/', {
    headers: {"authorization": "Bearer SOMETHING"},
    timeout: 1000
  });

  console.log('END');

  server.close();
})();

First, thanks for your reply.

However, I have been isolating the issue and I figure out that it is an issue related with got and Node 10.15.1 version. In newer Node version, got 11 works correctly. The interesting things is that using got in version 10.7.0 works also in Node 10.15.1.

In this repo I have reproduced the problem.

[Giotino]

I found something.

This issue has nothing to do with the authorization header, but since it's echoed in the response body it can change the behavior.

The thing is that got seems to have problem with the gzip compression. The postman echo decides to use gzip only if the size of the body exceed a certain size, with express I found that the minimum body size is 1Kb.

Code to reproduce

import got from "../source";
import express = require("express");
const compression = require('compression');

const app = express();
app.use(compression({ filter: (_req: any, _res: any) => true }));
app.get("/", (_req: any, res: any, _next: any) => {
  let body = '';
  for(let i=0; i<1024; i++) body += 'a';
  res.end(body);
});

(async () => {
  const server = app.listen(4444);
  
	try {
		await got("http://127.0.0.1:4444");
		console.log("END");
	} catch {
		console.log("ERROR");
	}

	server.close();
})();

A workaround could be removing the compression await got({decompress: false});

[Giotino]

Since it's not happening on Node.JS v10.21.0 that's the latest 10 available, nor on the v10.15.3 I would conclude that's some kind of bug of Node.JS v10.15.1.

TL;DR Update Node.JS

[szmarczak]

I can reproduce #1287 (comment) on 11.4.0 and Node.js 14.5.0.

[szmarczak]

Indeed, it is working as expected on 10.7.0.

[szmarczak]

So I figured out what happens. It sends a request to verify the cache, so request event is emitted, and in case the cached response is valid, it's reused, therefore the req property is missing.