From eead318625347bb9de8f9d7ffc6fae5ae1b33901 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?TZ=20=7C=20=E5=A4=A9=E7=8C=AA?= Date: Thu, 11 Apr 2019 17:21:52 +0800 Subject: [PATCH] feat: switch httpclient to httpclient2 for retry feature (#3606) --- app/extend/context.js | 2 +- app/extend/helper.js | 4 +-- app/extend/request.js | 18 ++++++++++--- app/extend/response.js | 12 ++++++++- index.d.ts | 12 ++++----- lib/agent.js | 2 +- lib/application.js | 2 +- lib/core/base_context_logger.js | 6 ++++- lib/core/httpclient.js | 2 +- lib/egg.js | 4 +-- test/app/extend/context.test.js | 4 +-- .../apps/httpclient-retry/package.json | 3 +++ test/lib/core/httpclient.test.js | 27 +++++++++++++++++++ test/utils.js | 11 ++++++++ 14 files changed, 87 insertions(+), 22 deletions(-) create mode 100644 test/fixtures/apps/httpclient-retry/package.json diff --git a/app/extend/context.js b/app/extend/context.js index ed8041e471..87b0b7d9e9 100644 --- a/app/extend/context.js +++ b/app/extend/context.js @@ -39,7 +39,7 @@ const proto = module.exports = { /** * Shortcut for httpclient.curl * - * @method Context#curl + * @function Context#curl * @param {String|Object} url - request url address. * @param {Object} [options] - options for request. * @return {Object} see {@link ContextHttpClient#curl} diff --git a/app/extend/helper.js b/app/extend/helper.js index 90e0d8064c..3120944d25 100644 --- a/app/extend/helper.js +++ b/app/extend/helper.js @@ -7,7 +7,7 @@ module.exports = { /** * Generate URL path(without host) for route. Takes the route name and a map of named params. - * @method Helper#pathFor + * @function Helper#pathFor * @param {String} name - Router Name * @param {Object} params - Other params * @@ -25,7 +25,7 @@ module.exports = { /** * Generate full URL(with host) for route. Takes the route name and a map of named params. - * @method Helper#urlFor + * @function Helper#urlFor * @param {String} name - Router name * @param {Object} params - Other params * @example diff --git a/app/extend/request.js b/app/extend/request.js index 407b9249bf..374185046f 100644 --- a/app/extend/request.js +++ b/app/extend/request.js @@ -95,9 +95,9 @@ module.exports = { }, /** - * Get or set the request remote IPv4 address + * Get the request remote IPv4 address * @member {String} Request#ip - * @param {String} ip - IPv4 address + * @return {String} IPv4 address * @example * ```js * this.request.ip @@ -116,6 +116,17 @@ module.exports = { return this._ip; }, + /** + * Set the request remote IPv4 address + * @member {String} Request#ip + * @param {String} ip - IPv4 address + * @example + * ```js + * this.request.ip + * => '127.0.0.1' + * => '111.10.2.1' + * ``` + */ set ip(ip) { this._ip = ip; }, @@ -226,10 +237,9 @@ module.exports = { /** * Set query-string as an object. * - * @method Request#query + * @function Request#query * @param {Object} obj set querystring and query object for request. * @return {void} - * @api public */ set query(obj) { this.querystring = querystring.stringify(obj); diff --git a/app/extend/response.js b/app/extend/response.js index 4695a46697..643d53f610 100644 --- a/app/extend/response.js +++ b/app/extend/response.js @@ -76,7 +76,7 @@ module.exports = { * And access log should save 500 not 302, * then the `realStatus` can help us find out the real status code. * @member {Number} Response#realStatus - * @param {Number} status The status code to be set. + * @return {Number} The status code to be set. */ get realStatus() { if (this[REAL_STATUS]) { @@ -85,6 +85,16 @@ module.exports = { return this.status; }, + /** + * Set a real status code. + * + * e.g.: Using 302 status redirect to the global error page + * instead of show current 500 status page. + * And access log should save 500 not 302, + * then the `realStatus` can help us find out the real status code. + * @member {Number} Response#realStatus + * @param {Number} status The status code to be set. + */ set realStatus(status) { this[REAL_STATUS] = status; }, diff --git a/index.d.ts b/index.d.ts index 1997f2b5a6..5193b52335 100644 --- a/index.d.ts +++ b/index.d.ts @@ -7,13 +7,13 @@ 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 { HttpClient2, RequestOptions2 as RequestOptions } from 'urllib'; import { EggCoreBase, FileLoaderOption, - EggLoader as CoreLoader, - EggCoreOptions as CoreOptions, - EggLoaderOptions as CoreLoaderOptions, + EggLoader as CoreLoader, + EggCoreOptions as CoreOptions, + EggLoaderOptions as CoreLoaderOptions, BaseContextClass as CoreBaseContextClass, } from 'egg-core'; import EggCookies = require('egg-cookies'); @@ -636,7 +636,7 @@ declare module 'egg' { truncated: boolean; } - + interface GetFileStreamOptions { requireFile?: boolean; // required file submit, default is true defCharset?: string; @@ -664,7 +664,7 @@ declare module 'egg' { * special properties (e.g: encrypted). So we must remove this property and * create our own with the same name. * @see https://github.com/eggjs/egg/pull/2958 - * + * * However, the latest version of Koa has "[key: string]: any" on the * context, and there'll be a type error for "keyof koa.Context". * So we have to directly inherit from "KoaApplication.BaseContext" and diff --git a/lib/agent.js b/lib/agent.js index 6259523f14..5dbbb8d106 100644 --- a/lib/agent.js +++ b/lib/agent.js @@ -14,7 +14,7 @@ const EGG_PATH = Symbol.for('egg#eggPath'); */ class Agent extends EggApplication { /** - * @constructor + * @class * @param {Object} options - see {@link EggApplication} */ constructor(options = {}) { diff --git a/lib/application.js b/lib/application.js index 4f34c42261..75cff1f420 100644 --- a/lib/application.js +++ b/lib/application.js @@ -52,7 +52,7 @@ function escapeHeaderValue(value) { class Application extends EggApplication { /** - * @constructor + * @class * @param {Object} options - see {@link EggApplication} */ constructor(options = {}) { diff --git a/lib/core/base_context_logger.js b/lib/core/base_context_logger.js index f4a8a3b551..64baff541d 100644 --- a/lib/core/base_context_logger.js +++ b/lib/core/base_context_logger.js @@ -5,7 +5,7 @@ const CALL = Symbol('BaseContextLogger#call'); class BaseContextLogger { /** - * @constructor + * @class * @param {Context} ctx - context instance * @param {String} pathName - class path name * @since 1.0.0 @@ -29,6 +29,7 @@ class BaseContextLogger { /** * @member {Function} BaseContextLogger#debug + * @param {...any} args - log msg * @since 1.2.0 */ debug(...args) { @@ -37,6 +38,7 @@ class BaseContextLogger { /** * @member {Function} BaseContextLogger#info + * @param {...any} args - log msg * @since 1.2.0 */ info(...args) { @@ -45,6 +47,7 @@ class BaseContextLogger { /** * @member {Function} BaseContextLogger#warn + * @param {...any} args - log msg * @since 1.2.0 */ warn(...args) { @@ -53,6 +56,7 @@ class BaseContextLogger { /** * @member {Function} BaseContextLogger#error + * @param {...any} args - log msg * @since 1.2.0 */ error(...args) { diff --git a/lib/core/httpclient.js b/lib/core/httpclient.js index 2ed61b044c..4e1400b0c0 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; diff --git a/lib/egg.js b/lib/egg.js index 96c978b534..b4421e4654 100644 --- a/lib/egg.js +++ b/lib/egg.js @@ -33,7 +33,7 @@ const CLUSTER_CLIENTS = Symbol.for('egg#clusterClients'); class EggApplication extends EggCore { /** - * @constructor + * @class * @param {Object} options * - {Object} [type] - type of instance, Agent and Application both extend koa, type can determine what it is. * - {String} [baseDir] - app root dir, default is `process.cwd()` @@ -508,7 +508,7 @@ class EggApplication extends EggCore { /** * Create egg context - * @method EggApplication#createContext + * @function EggApplication#createContext * @param {Req} req - node native Request object * @param {Res} res - node native Response object * @return {Context} context object diff --git a/test/app/extend/context.test.js b/test/app/extend/context.test.js index 8d79b0dd94..5cf7267112 100644 --- a/test/app/extend/context.test.js +++ b/test/app/extend/context.test.js @@ -293,8 +293,8 @@ describe('test/app/extend/context.test.js', () => { .get('/error') .expect(200) .expect('hello error'); - assert(errorHadEmit); await sleep(5000); + assert(errorHadEmit); const logdir = app.config.logger.dir; const log = fs.readFileSync(path.join(logdir, 'common-error.log'), 'utf8'); assert(/ENOENT: no such file or directory/.test(log)); @@ -359,8 +359,8 @@ describe('test/app/extend/context.test.js', () => { .get('/error') .expect(200) .expect('hello error'); - assert(errorHadEmit); await sleep(5000); + assert(errorHadEmit); const logdir = app.config.logger.dir; const log = fs.readFileSync(path.join(logdir, 'common-error.log'), 'utf8'); assert(/ENOENT: no such file or directory/.test(log)); 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..c5787b4b6a 100644 --- a/test/lib/core/httpclient.test.js +++ b/test/lib/core/httpclient.test.js @@ -417,4 +417,31 @@ 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); + }); + }); }); diff --git a/test/utils.js b/test/utils.js index 8f5fac1a9e..4a3b2f9b3b 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,16 @@ exports.startLocalServer = () => { res.end(`${req.method} ${req.url}`); }, 10000); return; + } else if (req.url === '/retry') { + if (!retry) { + retry = true; + res.statusCode = 500; + res.end(); + } else { + res.statusCode = 200; + res.end('retry suc'); + } + return; } else { res.end(`${req.method} ${req.url}`); }