Skip to content

Commit

Permalink
Fix hanging promise when using cache
Browse files Browse the repository at this point in the history
Fixes #1193
  • Loading branch information
szmarczak committed May 2, 2020
1 parent 633e46e commit 7b19e8f
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 16 deletions.
39 changes: 23 additions & 16 deletions source/core/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1160,7 +1160,9 @@ export default class Request extends Duplex implements RequestEvents<Request> {

this[kCancelTimeouts] = timedOut(request, timeout, url);

request.once('response', response => {
const responseEventName = options.cache ? 'cacheableResponse' : 'response';

request.once(responseEventName, (response: IncomingMessage) => {
this._onResponse(response);
});

Expand Down Expand Up @@ -1220,19 +1222,21 @@ export default class Request extends Duplex implements RequestEvents<Request> {
delete (options as unknown as NormalizedOptions).url;

// This is ugly
const cacheRequest = cacheableStore.get((options as any).cache)!(options, resolve as any);

// Restore options
(options as unknown as NormalizedOptions).url = url;
const cacheRequest = cacheableStore.get((options as any).cache)!(options, response => {
const typedResponse = response as unknown as IncomingMessage & {req: ClientRequest};
const {req} = typedResponse;

cacheRequest.once('error', (error: Error) => {
if (error instanceof CacheableRequest.CacheError) {
reject(new CacheError(error, this));
return;
if (req) {
req.emit('cacheableResponse', typedResponse);
}

reject(error);
resolve(typedResponse as unknown as ResponseLike);
});

// Restore options
(options as unknown as NormalizedOptions).url = url;

cacheRequest.once('error', reject);
cacheRequest.once('request', resolve);
});
}
Expand Down Expand Up @@ -1295,6 +1299,7 @@ export default class Request extends Duplex implements RequestEvents<Request> {

const isHttps = url.protocol === 'https:';

// Fallback function
let fallbackFn: HttpRequestFunction;
if (options.http2) {
fallbackFn = http2wrapper.auto;
Expand All @@ -1303,20 +1308,22 @@ export default class Request extends Duplex implements RequestEvents<Request> {
}

const realFn = options.request ?? fallbackFn;
const fn = options.cache ? this._createCacheableRequest.bind(this) : realFn;

// Cache support
const fn = options.cache ? this._createCacheableRequest : realFn;

// Pass an agent directly when HTTP2 is disabled
if (agent && !options.http2) {
(options as unknown as RequestOptions).agent = agent[isHttps ? 'https' : 'http'];
}

// Prepare plain HTTP request options
options[kRequest] = realFn as HttpRequestFunction;
delete options.request;
delete options.timeout;

let requestOrResponse: ReturnType<RequestFunction>;

try {
requestOrResponse = await fn(url, options as unknown as RequestOptions);
let requestOrResponse = await fn(url, options as unknown as RequestOptions);

if (is.undefined(requestOrResponse)) {
requestOrResponse = fallbackFn(url, options as unknown as RequestOptions);
Expand All @@ -1343,8 +1350,8 @@ export default class Request extends Duplex implements RequestEvents<Request> {
this._onResponse(requestOrResponse as IncomingMessage);
}
} catch (error) {
if (error instanceof RequestError) {
throw error;
if (error instanceof CacheableRequest.CacheError) {
throw new CacheError(error, this);
}

throw new RequestError(error.message, error, this);
Expand Down
25 changes: 25 additions & 0 deletions test/cache.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {Handler} from 'express';
import {Response} from '../source';
import withServer from './helpers/with-server';
import CacheableLookup from 'cacheable-lookup';
import delay = require('delay');

const cacheEndpoint: Handler = (_request, response) => {
response.setHeader('Cache-Control', 'public, max-age=60');
Expand Down Expand Up @@ -285,3 +286,27 @@ test('can replace the instance\'s HTTP cache', withServer, async (t, server, got
t.is(cache.size, 1);
t.is(secondCache.size, 1);
});

test('does not hang on huge response', withServer, async (t, server, got) => {
const bufferSize = 3 * 16 * 1024;
const times = 10;

const buffer = Buffer.alloc(bufferSize);

server.get('/', async (_request, response) => {
for (let i = 0; i < 10; i++) {
response.write(buffer);

// eslint-disable-next-line no-await-in-loop
await delay(100);
}

response.end();
});

const body = await got('', {
cache: new Map()
}).buffer();

t.is(body.length, bufferSize * times);
});

0 comments on commit 7b19e8f

Please sign in to comment.