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.

Sign up for GitHub

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

feat: timeout option #201

Merged
merged 5 commits into from
Aug 8, 2016
Merged

feat: timeout option #201

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
593f877
feat: timeout option
ibigbug Aug 4, 2016
a0f14df
fix: socket full case
ibigbug Aug 5, 2016
4c03e85
style: code style
ibigbug Aug 5, 2016
5f53b81
fix: do not break public API
ibigbug Aug 5, 2016
037509d
doc: update timeout jsdoc
ibigbug Aug 8, 2016
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
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ httpclient.request('http://nodejs.org', function (err, body) {
- ***dataType*** String - Type of response data. Could be `text` or `json`. If it's `text`, the `callback`ed `data` would be a String. If it's `json`, the `data` of callback would be a parsed JSON Object. Default `callback`ed `data` would be a `Buffer`.
- **fixJSONCtlChars** Boolean - Fix the control characters (U+0000 through U+001F) before JSON parse response. Default is `false`.
- ***headers*** Object - Request headers.
- ***timeout*** Number - Request timeout in milliseconds. Defaults to `exports.TIMEOUT`. Include remote server connecting timeout and response timeout. When timeout happen, will return `ConnectionTimeout` or `ResponseTimeout`.
- ***timeout*** Number | Array - Request timeout in milliseconds for connecting phase and response receiving phase. Defaults to `exports.TIMEOUT`, both are 5s. You can use `timeout: 5000` to tell urllib use same timeout on two phase or set them seperately such as `timeout: [3000, 5000]`, which will set connecting timeout to 3s and response 5s.
Copy link
Member

Choose a reason for hiding this comment

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

感觉变化了，原来是 5s 包含2个过程，现在变成了每个过程 5s。不过也还好。

- ***auth*** String - `username:password` used in HTTP Basic Authorization.
- ***digestAuth*** String - `username:password` used in HTTP [Digest Authorization](http://en.wikipedia.org/wiki/Digest_access_authentication).
- ***agent*** [http.Agent](http://nodejs.org/api/http.html#http_class_http_agent) - HTTP Agent object.
Expand Down
113 changes: 80 additions & 33 deletions lib/urllib.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ exports.httpsAgent.maxSockets = 1000;
*/

exports.TIMEOUT = ms('5s');
exports.TIMEOUTS = [ms('5s'), ms('5s')];

var REQUEST_ID = 0;
var MAX_VALUE = Math.pow(2, 31) - 10;
Expand Down Expand Up @@ -189,7 +190,7 @@ exports.requestWithCallback = function (url, args, callback) {
});
}

args.timeout = args.timeout || exports.TIMEOUT;
args.timeout = args.timeout || exports.TIMEOUTS;
args.maxRedirects = args.maxRedirects || 10;
args.streaming = args.streaming || args.customResponse;
var requestStartTime = Date.now();
Expand Down Expand Up @@ -315,17 +316,29 @@ exports.requestWithCallback = function (url, args, callback) {
// you can use this hook to change every thing.
args.beforeRequest(options);
}
var timer = null;
var connectTimer = null;
var responseTimer = null;
var __err = null;
var connected = false; // socket connected or not
var keepAliveSocket = false; // request with keepalive socket
var responseSize = 0;
var responseAborted = false;
var done = function (err, data, res) {
if (timer) {
clearTimeout(timer);
timer = null;

function cancelConnectTimer() {
if (connectTimer) {
clearTimeout(connectTimer);
connectTimer = null;
}
}
function cancelResponseTimer() {
if (responseTimer) {
clearTimeout(responseTimer);
responseTimer = null;
}
}

function done(err, data, res) {
cancelResponseTimer();
if (!callback) {
console.warn('[urllib:warn] [%s] [worker:%s] %s %s callback twice!!!',
Date(), process.pid, options.method, url);
Expand Down Expand Up @@ -409,9 +422,9 @@ exports.requestWithCallback = function (url, args, callback) {
res: response
});
}
};
}

var handleRedirect = function (res) {
function handleRedirect(res) {
var err = null;
if (args.followRedirect && statuses.redirect[res.statusCode]) { // handle redirect
args._followRedirectCount = (args._followRedirectCount || 0) + 1;
Copy link
Member

Choose a reason for hiding this comment

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

原来触发多次 callback 的问题就在这两个 error

Expand All @@ -426,10 +439,7 @@ exports.requestWithCallback = function (url, args, callback) {
var _url = urlutil.resolve(url, location);
debug('Request#%d %s: `redirected` from %s to %s', reqId, options.path, url, _url);
// make sure timer stop
if (timer) {
clearTimeout(timer);
timer = null;
}
cancelResponseTimer();
// should clean up headers.Host on `location: http://other-domain/url`
Copy link
Member

Choose a reason for hiding this comment

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

看来原来的实现由 bug，MaxRedirectError 和 FollowRedirectError 的逻辑也得cancelResponseTimer

if (args.headers && args.headers.Host && PROTO_RE.test(location)) {
args.headers.Host = null;
Expand All @@ -446,7 +456,7 @@ exports.requestWithCallback = function (url, args, callback) {
redirect: false,
error: err
};
};
}

// set user-agent
if (!options.headers['User-Agent'] && !options.headers['user-agent']) {
Expand All @@ -459,7 +469,7 @@ exports.requestWithCallback = function (url, args, callback) {
}
}

var decodeContent = function (res, body, cb) {
function decodeContent(res, body, cb) {
var encoding = res.headers['content-encoding'];
if (body.length === 0) {
return cb(null, body, encoding);
Expand All @@ -471,7 +481,7 @@ exports.requestWithCallback = function (url, args, callback) {

debug('gunzip %d length body', body.length);
zlib.gunzip(body, cb);
};
}

var writeStream = args.writeStream;

Expand Down Expand Up @@ -630,45 +640,78 @@ exports.requestWithCallback = function (url, args, callback) {
});
}

var connectTimeout, responseTimeout;
if (Array.isArray(args.timeout)) {
connectTimeout = ms(args.timeout[0]);
Copy link
Member

Choose a reason for hiding this comment

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

这里不对，不需要再使用 ms parse 一次的，这里的 timeout 只支持 number 格式，不支持字符串的。

Copy link
Member Author

Choose a reason for hiding this comment

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

额，如果传了 ['3s', '5s'] 进来还是可以的吧 ?

Copy link
Member

Choose a reason for hiding this comment

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

不支持，原来就不支持这种格式。

Copy link
Member Author

Choose a reason for hiding this comment

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

现在不是支持两种么，一种是原来的指传一个时间，现在可以传一个或两个

Copy link
Member

Choose a reason for hiding this comment

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

但是timeout也是number，不是string啊

Copy link
Member Author

Choose a reason for hiding this comment

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

额，明白了。ms 只是在内部处理用的。

Copy link
Member

Choose a reason for hiding this comment

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

好吧，原来也有这个逻辑。

responseTimeout = ms(args.timeout[1]);
} else { // set both timeout equal
connectTimeout = responseTimeout = ms(args.timeout);
}
debug('ConnectTimeout: %d, ResponseTimeout: %d', connectTimeout, responseTimeout);

function startConnectTimer() {
debug('Connect timer ticking, timeout: %d', connectTimeout);
connectTimer = setTimeout(function () {
connectTimer = null;
var msg = 'Connect timeout for ' + connectTimeout + 'ms';
var errorName = 'ConnectionTimeoutError';
if (!req.socket) {
errorName = 'SocketAssignTimeoutError';
msg += ', working sockets is full';
}
__err = new Error(msg);
__err.name = errorName;
__err.requestId = reqId;
debug('connectTimer boom!!!!');
debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
Copy link
Member

Choose a reason for hiding this comment

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

2个 debug 合并在一起吧

abortRequest();
}, connectTimeout);
}

function startResposneTimer() {
debug('Response timer ticking, timeout: %d', responseTimeout);
responseTimer = setTimeout(function () {
responseTimer = null;
var msg = 'Response timeout for ' + responseTimeout + 'ms';
var errorName = 'ResponseTimeoutError';
__err = new Error(msg);
__err.name = errorName;
__err.requestId = reqId;
debug('responseTimer boom!!!!');
debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
abortRequest();
}, responseTimeout);
}

var req;
// request headers checker will throw error
try {
req = httplib.request(options, onResponse);
} catch (err) {
return done(err);
}
// start connect timer just after `request` return
startConnectTimer();

var abortRequest = function () {
function abortRequest() {
debug('Request#%d %s abort, connected: %s', reqId, url, connected);
// it wont case error event when req haven't been assigned a socket yet.
if (!req.socket) {
__err.noSocket = true;
done(__err);
}
req.abort();
};

var timeout = ms(args.timeout);
}

timer = setTimeout(function () {
timer = null;
var msg = 'Request timeout for ' + timeout + 'ms';
var errorName = connected ? 'ResponseTimeoutError' : 'ConnectionTimeoutError';
if (!req.socket) {
errorName = 'SocketAssignTimeoutError';
msg += ', working sockets is full';
}
__err = new Error(msg);
__err.name = errorName;
__err.requestId = reqId;
debug('Request#%d %s %s: %s, connected: %s', reqId, url, __err.name, msg, connected);
abortRequest();
}, timeout);

req.once('socket', function (socket) {
// https://github.com/iojs/io.js/blob/v1.x/lib/net.js#L342
if (socket.readyState === 'opening') {
socket.once('connect', function () {
// cancel socket timer at first and start tick for TTFB
cancelConnectTimer();
startResposneTimer();

debug('Request#%d %s new socket connected', reqId, url);
connected = true;
});
Expand All @@ -678,6 +721,10 @@ exports.requestWithCallback = function (url, args, callback) {
debug('Request#%d %s reuse socket connected', reqId, url);
connected = true;
keepAliveSocket = true;

// reuse socket, timer should be canceled.
cancelConnectTimer();
startResposneTimer();
});

req.on('error', function (err) {
Expand Down
30 changes: 28 additions & 2 deletions test/urllib.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ describe('test/urllib.test.js', function () {
urllib.request('http://npm.taobao.org', { timeout: 1 }, function (err, data, res) {
should.exist(err);
err.name.should.equal('ConnectionTimeoutError');
err.message.should.match(/^Request timeout for 1ms\, GET http/);
err.message.should.match(/^Connect timeout for 1ms\, GET http/);
err.requestId.should.be.instanceof(Number);
should.not.exist(data);
should.exist(res);
Expand All @@ -272,7 +272,33 @@ describe('test/urllib.test.js', function () {
urllib.request(host + '/response_timeout', { timeout: 450 }, function (err, data, res) {
should.exist(err);
err.name.should.equal('ResponseTimeoutError');
err.message.should.match(/^Request timeout for 450ms\, GET http/);
err.message.should.match(/^Response timeout for 450ms\, GET http/);
err.requestId.should.be.instanceof(Number);
should.exist(data);
data.toString().should.equal('foo');
should.exist(res);
res.should.status(200);
done();
});
});

it('can pass two timeout seperately and get connect error', function (done) {
urllib.request('http://npm.taobao.net', { timeout: [1, 10000] }, function (err, data, res) {
should.exist(err);
err.name.should.equal('ConnectionTimeoutError');
err.message.should.match(/^Connect timeout for 1ms\, GET http/);
err.requestId.should.be.instanceof(Number);
should.not.exist(data);
should.exist(res);
done();
});
});

it('can pass two timeout seperately and get response error', function(done) {
urllib.request(host + '/response_timeout', { timeout: [1000, 450] }, function (err, data, res) {
should.exist(err);
err.name.should.equal('ResponseTimeoutError');
err.message.should.match(/^Response timeout for 450ms\, GET http/);
err.requestId.should.be.instanceof(Number);
should.exist(data);
data.toString().should.equal('foo');
Expand Down