diff --git a/index.d.ts b/index.d.ts index fac192b06c..0597b05885 100644 --- a/index.d.ts +++ b/index.d.ts @@ -7,7 +7,7 @@ import { Readable } from 'stream'; import { Socket } from 'net'; import { IncomingMessage, ServerResponse } from 'http'; import { EggLogger, EggLoggers, LoggerLevel as EggLoggerLevel, EggContextLogger } from 'egg-logger'; -import { HttpClient2, RequestOptions } from 'urllib'; +import { HttpClient, RequestOptions2 as RequestOptions } from 'urllib'; import { EggCoreBase, FileLoaderOption, @@ -37,12 +37,12 @@ declare module 'egg' { // Remove specific property from the specific class type RemoveSpecProp = Pick>; - interface EggHttpClient extends HttpClient2 {} + interface EggHttpClient extends HttpClient {} interface EggHttpConstructor { new (app: Application): EggHttpClient; } - interface EggContextHttpClient extends HttpClient2 {} + interface EggContextHttpClient extends HttpClient {} interface EggContextHttpClientConstructor { new (ctx: Context): EggContextHttpClient; } diff --git a/lib/core/dnscache_httpclient.js b/lib/core/dnscache_httpclient.js index 61bce79271..04dc1f91e1 100644 --- a/lib/core/dnscache_httpclient.js +++ b/lib/core/dnscache_httpclient.js @@ -43,27 +43,6 @@ class DNSCacheHttpClient extends HttpClient { })(); } - curl(url, args, callback) { - return this.request(url, args, callback); - } - - requestThunk(url, args) { - this.app.deprecate('[dnscache_httpclient] Please use `request()` instead of `requestThunk()`'); - return callback => { - this.request(url, args, (err, data, res) => { - if (err) { - return callback(err); - } - callback(null, { - data, - status: res.status, - headers: res.headers, - res, - }); - }); - }; - } - async [DNSLOOKUP](url, args) { const parsed = typeof url === 'string' ? urlparse(url) : url; // hostname must exists diff --git a/lib/core/httpclient.js b/lib/core/httpclient.js index 2ed61b044c..9151ce6fae 100644 --- a/lib/core/httpclient.js +++ b/lib/core/httpclient.js @@ -5,7 +5,7 @@ const HttpsAgent = require('agentkeepalive').HttpsAgent; const urllib = require('urllib'); const ms = require('humanize-ms'); -class HttpClient extends urllib.HttpClient { +class HttpClient extends urllib.HttpClient2 { constructor(app) { normalizeConfig(app); const config = app.config.httpclient; @@ -32,7 +32,38 @@ class HttpClient extends urllib.HttpClient { args.tracer = args.tracer || this.app.tracer; } - return super.request(url, args, callback); + // the callback style + if (callback) { + this.app.deprecate('[httpclient] We now support async for this function, so callback isn\'t recommended.'); + super.request(url, args) + .then(result => process.nextTick(() => callback(null, result.data, result.res))) + .catch(err => process.nextTick(() => callback(err))); + return; + } + + // the Promise style + return super.request(url, args); + } + + curl(url, args, callback) { + return this.request(url, args, callback); + } + + requestThunk(url, args) { + this.app.deprecate('[httpclient] Please use `request()` instead of `requestThunk()`'); + return callback => { + this.request(url, args, (err, data, res) => { + if (err) { + return callback(err); + } + callback(null, { + data, + status: res.status, + headers: res.headers, + res, + }); + }); + }; } } diff --git a/test/fixtures/apps/httpclient-retry/package.json b/test/fixtures/apps/httpclient-retry/package.json new file mode 100644 index 0000000000..eb178c25f0 --- /dev/null +++ b/test/fixtures/apps/httpclient-retry/package.json @@ -0,0 +1,3 @@ +{ + "name": "httpclient-retry" +} \ No newline at end of file diff --git a/test/lib/core/httpclient.test.js b/test/lib/core/httpclient.test.js index dfd99a4d37..245e404c36 100644 --- a/test/lib/core/httpclient.test.js +++ b/test/lib/core/httpclient.test.js @@ -11,6 +11,7 @@ describe('test/lib/core/httpclient.test.js', () => { before(() => { client = new Httpclient({ + deprecate: () => {}, config: { httpclient: { request: {}, @@ -54,6 +55,13 @@ describe('test/lib/core/httpclient.test.js', () => { client.request(url, () => {}); }); + it('should request callback with error', done => { + client.request(url + '/error', { dataType: 'json' }, err => { + assert(err); + done(); + }); + }); + it('should curl ok with log', done => { const args = { dataType: 'text', @@ -417,4 +425,111 @@ describe('test/lib/core/httpclient.test.js', () => { assert(!mockApp.config.httpclient.httpsAgent.freeSocketKeepAliveTimeout); }); }); + + describe('httpclient retry', () => { + let app; + before(() => { + app = utils.app('apps/httpclient-retry'); + return app.ready(); + }); + after(() => app.close()); + + it('should retry when httpclient fail', async () => { + let hasRetry = false; + const res = await app.httpclient.curl(`${url}/retry`, { + retry: 1, + retryDelay: 100, + isRetry(res) { + const shouldRetry = res.status >= 500; + if (shouldRetry) { + hasRetry = true; + } + return shouldRetry; + }, + }); + + assert(hasRetry); + assert(res.status === 200); + }); + + it('should callback style retry when httpclient fail', done => { + let hasRetry = false; + app.httpclient.request(`${url}/retry`, { + retry: 1, + retryDelay: 100, + isRetry(res) { + const shouldRetry = res.status >= 500; + if (shouldRetry) { + hasRetry = true; + } + return shouldRetry; + }, + }, (err, data, res) => { + assert(hasRetry); + assert(res.status === 200); + assert(data.toString() === 'retry suc'); + done(err); + }); + }); + + it('should retry when httpclient fail', async () => { + let hasRetry = false; + const res = await app.httpclient.curl(`${url}/retry`, { + retry: 1, + retryDelay: 100, + isRetry(res) { + const shouldRetry = res.status >= 500; + if (shouldRetry) { + hasRetry = true; + } + return shouldRetry; + }, + }); + + assert(hasRetry); + assert(res.status === 200); + }); + + it('should callback style retry when httpclient fail', done => { + let hasRetry = false; + app.httpclient.request(`${url}/retry`, { + retry: 1, + retryDelay: 100, + isRetry(res) { + const shouldRetry = res.status >= 500; + if (shouldRetry) { + hasRetry = true; + } + return shouldRetry; + }, + }, (err, data, res) => { + assert(hasRetry); + assert(res.status === 200); + assert(data.toString() === 'retry suc'); + done(err); + }); + }); + + it('should thunk style retry when httpclient fail', done => { + let hasRetry = false; + app.httpclient.requestThunk(`${url}/retry`, { + retry: 1, + retryDelay: 100, + isRetry(res) { + const shouldRetry = res.status >= 500; + if (shouldRetry) { + hasRetry = true; + } + return shouldRetry; + }, + })((err, { data, status, headers, res }) => { + assert(hasRetry); + assert(status === 200); + assert(res.status === 200); + assert(data.toString() === 'retry suc'); + assert(headers['x-retry'] === '1'); + done(err); + }); + }); + }); }); diff --git a/test/utils.js b/test/utils.js index 8f5fac1a9e..1e515f6494 100644 --- a/test/utils.js +++ b/test/utils.js @@ -50,6 +50,7 @@ exports.startLocalServer = () => { if (localServer) { return resolve('http://127.0.0.1:' + localServer.address().port); } + let retry = false; localServer = http.createServer((req, res) => { req.resume(); req.on('end', () => { @@ -62,6 +63,22 @@ exports.startLocalServer = () => { res.end(`${req.method} ${req.url}`); }, 10000); return; + } else if (req.url === '/error') { + res.statusCode = 500; + res.end('this is an error'); + return; + } else if (req.url === '/retry') { + if (!retry) { + retry = true; + res.statusCode = 500; + res.end(); + } else { + res.setHeader('x-retry', '1'); + res.statusCode = 200; + res.end('retry suc'); + retry = false; + } + return; } else { res.end(`${req.method} ${req.url}`); }