Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Customize timeouts and generally improve the whole thing #534

Merged
merged 7 commits into from
Jul 25, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,14 @@ Type: `number` `Object`

Milliseconds to wait for the server to end the response before aborting request with `ETIMEDOUT` error (a.k.a. `request` property). By default there's no timeout.

This also accepts an object with separate `connect`, `socket`, and `request` fields for connection, socket, and entire request timeouts.
This also accepts an object with separate `lookup`, `connect`, `socket`, `response` and `request` fields to specify granular timeouts for each phase of the request.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Being able to set timeouts around each phase is more useful that timing each phase from the beginning of the request.

A concrete example is being able to time the response phase (TTFB) when the target is a load-balanced java service which may be undergoing garbage collection. We often want to set a very aggressive response timeout in this case, so that we can retry the request immediately against another instance.


- `lookup` starts when a socket is assigned and ends when the hostname has been resolved. Does not apply when using a Unix domain socket.
- `connect` starts when `lookup` completes (or when the socket is assigned if lookup does not apply to the request) and ends when the socket is connected.
- `socket` starts when the socket is connected. See [request.setTimeout](https://nodejs.org/api/http.html#http_request_settimeout_timeout_callback).
- `response` starts when the request has been written to the socket and ends when the response headers are received.
- `send` starts when the socket is connected and ends with the request has been written to the socket.
- `request` starts when the request is initiated and ends when the response's end event fires.

###### retry

Expand Down
7 changes: 6 additions & 1 deletion source/as-promise.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,22 @@ module.exports = options => {

proxy.emit('request', req);

const uploadComplete = () => {
req.emit('upload-complete');
};

onCancel(() => {
req.abort();
});

if (is.nodeStream(options.body)) {
options.body.once('end', uploadComplete);
options.body.pipe(req);
options.body = undefined;
return;
}

req.end(options.body);
req.end(options.body, uploadComplete);
});

emitter.on('response', async response => {
Expand Down
9 changes: 7 additions & 2 deletions source/as-stream.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,23 +28,28 @@ module.exports = options => {

emitter.on('request', req => {
proxy.emit('request', req);
const uploadComplete = () => {
req.emit('upload-complete');
};

if (is.nodeStream(options.body)) {
options.body.once('end', uploadComplete);
options.body.pipe(req);
return;
}

if (options.body) {
req.end(options.body);
req.end(options.body, uploadComplete);
return;
}

if (options.method === 'POST' || options.method === 'PUT' || options.method === 'PATCH') {
input.once('end', uploadComplete);
input.pipe(req);
return;
}

req.end();
req.end(uploadComplete);
});

emitter.on('response', response => {
Expand Down
9 changes: 9 additions & 0 deletions source/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class GotError extends Error {
hostname: opts.hostname,
method: opts.method,
path: opts.path,
socketPath: opts.socketPath,
protocol: opts.protocol,
url: opts.href
});
Expand Down Expand Up @@ -89,4 +90,12 @@ module.exports.UnsupportedProtocolError = class extends GotError {
}
};

module.exports.TimeoutError = class extends GotError {
constructor(threshold, event, opts) {
super(`Timeout awaiting '${event}' for ${threshold}ms`, {code: 'ETIMEDOUT'}, opts);
this.name = 'TimeoutError';
this.event = event;
}
};

module.exports.CancelError = PCancelable.CancelError;
28 changes: 13 additions & 15 deletions source/request-as-event-emitter.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ const timedOut = require('./timed-out');
const getBodySize = require('./get-body-size');
const getResponse = require('./get-response');
const progress = require('./progress');
const {CacheError, UnsupportedProtocolError, MaxRedirectsError, RequestError} = require('./errors');
const {GotError, CacheError, UnsupportedProtocolError, MaxRedirectsError, RequestError} = require('./errors');

const getMethodRedirectCodes = new Set([300, 301, 302, 303, 304, 305, 307, 308]);
const allMethodRedirectCodes = new Set([300, 303, 307, 308]);
Expand Down Expand Up @@ -91,13 +91,11 @@ module.exports = (options = {}) => {
return;
}

setImmediate(() => {
try {
getResponse(response, options, emitter, redirects);
} catch (error) {
emitter.emit('error', error);
}
});
try {
getResponse(response, options, emitter, redirects);
} catch (error) {
emitter.emit('error', error);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we leave this unchanged as this is being discussed in #525? Try not to duplicate PRs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, this should not have been included here, but since I'm gonna accept #525, I'll allow it.

});

cacheReq.on('error', error => {
Expand All @@ -119,23 +117,23 @@ module.exports = (options = {}) => {
return;
}

const err = new RequestError(error, options);
emitter.emit('retry', err, retried => {
if (!(error instanceof GotError)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wasn't sure that this was the precise change needed. My objective was to expose the TimeoutError to a client using the Promise interface.

error = new RequestError(error, options);
}
emitter.emit('retry', error, retried => {
if (!retried) {
emitter.emit('error', err);
emitter.emit('error', error);
}
});
});

progress.upload(req, emitter, uploadBodySize);

if (options.gotTimeout) {
timedOut(req, options.gotTimeout);
timedOut(req, options);
}

setImmediate(() => {
emitter.emit('request', req);
});
emitter.emit('request', req);
});
};

Expand Down
178 changes: 115 additions & 63 deletions source/timed-out.js
Original file line number Diff line number Diff line change
@@ -1,79 +1,131 @@
'use strict';
const net = require('net');
const {TimeoutError} = require('./errors');

// Forked from https://github.com/floatdrop/timed-out
const reentry = Symbol('reentry');

module.exports = function (req, delays) {
if (req.timeoutTimer) {
return req;
function addTimeout(delay, callback, ...args) {
// Event loop order is timers, poll, immediates.
// The timed event may emit during the current tick poll phase, so
// defer calling the handler until the poll phase completes.
let immediate;
const timeout = setTimeout(
() => {
immediate = setImmediate(callback, delay, ...args);
if (immediate.unref) {
// Added in node v9.7.0
immediate.unref();
}
},
delay
);
timeout.unref();
return () => {
clearTimeout(timeout);
clearImmediate(immediate);
};
}

module.exports = function (req, options) {
if (req[reentry]) {
return;
}
req[reentry] = true;
const {gotTimeout: delays, host, hostname} = options;
const timeoutHandler = (delay, event) => {
req.abort();
req.emit('error', new TimeoutError(delay, event, options));
};
const cancelers = [];
const cancelTimeouts = () => {
cancelers.forEach(cancelTimeout => cancelTimeout());
};

const host = req._headers ? (' to ' + req._headers.host) : '';
req.on('error', cancelTimeouts);
req.once('response', response => {
response.once('end', cancelTimeouts);
});

function throwESOCKETTIMEDOUT() {
req.abort();
const e = new Error('Socket timed out on request' + host);
e.code = 'ESOCKETTIMEDOUT';
req.emit('error', e);
if (delays.request !== undefined) {
const cancelTimeout = addTimeout(
delays.request,
timeoutHandler,
'request'
);
cancelers.push(cancelTimeout);
}

function throwETIMEDOUT() {
req.abort();
const e = new Error('Connection timed out on request' + host);
e.code = 'ETIMEDOUT';
req.emit('error', e);
if (delays.socket !== undefined) {
req.setTimeout(
delays.socket,
() => {
timeoutHandler(delays.socket, 'socket');
}
);
}
if (delays.lookup !== undefined && !req.socketPath && !net.isIP(hostname || host)) {
req.once('socket', socket => {
if (socket.connecting) {
const cancelTimeout = addTimeout(
delays.lookup,
timeoutHandler,
'lookup'
);
cancelers.push(cancelTimeout);
socket.once('lookup', cancelTimeout);
}
});
}

if (delays.connect !== undefined) {
req.timeoutTimer = setTimeout(throwETIMEDOUT, delays.connect);
req.once('socket', socket => {
if (socket.connecting) {
const timeConnect = () => {
const cancelTimeout = addTimeout(
delays.connect,
timeoutHandler,
'connect'
);
cancelers.push(cancelTimeout);
return cancelTimeout;
};
if (req.socketPath || net.isIP(hostname || host)) {
socket.once('connect', timeConnect());
} else {
socket.once('lookup', () => {
socket.once('connect', timeConnect());
});
}
}
});
}

if (delays.request !== undefined) {
req.requestTimeoutTimer = setTimeout(() => {
clear();

if (req.connection.connecting) {
throwETIMEDOUT();
if (delays.send !== undefined) {
req.once('socket', socket => {
const timeRequest = () => {
const cancelTimeout = addTimeout(
delays.send,
timeoutHandler,
'send'
);
cancelers.push(cancelTimeout);
return cancelTimeout;
};
if (socket.connecting) {
socket.once('connect', () => {
req.once('upload-complete', timeRequest());
});
} else {
throwESOCKETTIMEDOUT();
req.once('upload-complete', timeRequest());
}
}, delays.request);
}

// Clear the connection timeout timer once a socket is assigned to the
// request and is connected.
req.on('socket', socket => {
// Socket may come from Agent pool and may be already connected.
if (!socket.connecting) {
connect();
return;
}

socket.once('connect', connect);
});

function clear() {
if (req.timeoutTimer) {
clearTimeout(req.timeoutTimer);
req.timeoutTimer = null;
}
});
}

function connect() {
clear();

if (delays.socket !== undefined) {
// Abort the request if there is no activity on the socket for more
// than `delays.socket` milliseconds.
req.setTimeout(delays.socket, throwESOCKETTIMEDOUT);
}

req.on('response', res => {
res.on('end', () => {
// The request is finished, cancel request timeout.
clearTimeout(req.requestTimeoutTimer);
});
if (delays.response !== undefined) {
req.once('upload-complete', () => {
const cancelTimeout = addTimeout(
delays.response,
timeoutHandler,
'response'
);
cancelers.push(cancelTimeout);
req.once('response', cancelTimeout);
});
}

return req.on('error', clear);
};
13 changes: 6 additions & 7 deletions test/retry.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ let fifth = 0;
let lastTried413access = Date.now();

const retryAfterOn413 = 2;
const connectTimeout = 500;
const socketTimeout = 100;
const socketTimeout = 200;

test.before('setup', async () => {
s = await createServer();
Expand Down Expand Up @@ -69,12 +68,12 @@ test.before('setup', async () => {
});

test('works on timeout error', async t => {
t.is((await got(`${s.url}/knock-twice`, {timeout: {connect: connectTimeout, socket: socketTimeout}})).body, 'who`s there?');
t.is((await got(`${s.url}/knock-twice`, {timeout: {socket: socketTimeout}})).body, 'who`s there?');
});

test('can be disabled with option', async t => {
const err = await t.throws(got(`${s.url}/try-me`, {
timeout: {connect: connectTimeout, socket: socketTimeout},
timeout: {socket: socketTimeout},
retry: 0
}));
t.truthy(err);
Expand All @@ -83,7 +82,7 @@ test('can be disabled with option', async t => {

test('function gets iter count', async t => {
await got(`${s.url}/fifth`, {
timeout: {connect: connectTimeout, socket: socketTimeout},
timeout: {socket: socketTimeout},
retry: {
retries: iteration => iteration < 10
}
Expand All @@ -93,7 +92,7 @@ test('function gets iter count', async t => {

test('falsy value prevents retries', async t => {
const err = await t.throws(got(`${s.url}/long`, {
timeout: {connect: connectTimeout, socket: socketTimeout},
timeout: {socket: socketTimeout},
retry: {
retries: () => 0
}
Expand All @@ -103,7 +102,7 @@ test('falsy value prevents retries', async t => {

test('falsy value prevents retries #2', async t => {
const err = await t.throws(got(`${s.url}/long`, {
timeout: {connect: connectTimeout, socket: socketTimeout},
timeout: {socket: socketTimeout},
retry: {
retries: (iter, err) => {
t.truthy(err);
Expand Down
Loading