From db4969ee1b14ac7898ed14b92c95f331e914ae07 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E5=86=B7=E8=8B=A5=E9=9C=9C=E5=AF=92?= <912881342@qq.com> Date: Thu, 26 Nov 2020 16:06:33 +0800 Subject: [PATCH] feat: retry when net err or timeout (#876) * feat: retry when net err or timeout * test: should not retry when params.stream is not readable * test: retry in browser Co-authored-by: beajer <919060679@qq.com> --- README.md | 1 + lib/browser/client.js | 23 +++++++++- lib/client.js | 24 +++++++++- lib/common/utils/retry.d.ts | 1 + lib/common/utils/retry.js | 30 ++++++++++++ lib/common/utils/retry.ts | 26 +++++++++++ package.json | 4 +- test/browser/browser.test.js | 82 +++++++++++++++++++++++++++++++-- test/node/utils/retry.test.js | 86 +++++++++++++++++++++++++++++++++++ 9 files changed, 269 insertions(+), 8 deletions(-) create mode 100644 lib/common/utils/retry.d.ts create mode 100644 lib/common/utils/retry.js create mode 100644 lib/common/utils/retry.ts create mode 100644 test/node/utils/retry.test.js diff --git a/README.md b/README.md index 7b0e175ef..b8687eb4f 100644 --- a/README.md +++ b/README.md @@ -336,6 +336,7 @@ options: `fetch` mode ,else `XMLHttpRequest` - [enableProxy] {Boolean}, Enable proxy request, default is false. - [proxy] {String | Object}, proxy agent uri or options, default is null. +- [retryMax] {Number}, used by auto retry send request count when request error is net error or timeout. example: diff --git a/lib/browser/client.js b/lib/browser/client.js index 0446aeebf..6f51d0a4f 100644 --- a/lib/browser/client.js +++ b/lib/browser/client.js @@ -13,6 +13,7 @@ const { createRequest } = require('../common/utils/createRequest'); const { encoder } = require('../common/utils/encoder'); const { getReqUrl } = require('../common/client/getReqUrl'); const { setSTSToken } = require('../common/utils/setSTSToken'); +const { retry } = require('../common/utils/retry'); const globalHttpAgent = new AgentKeepalive(); @@ -193,7 +194,27 @@ proto.authorization = function authorization(method, resource, subres, headers) * @api private */ -proto.request = async function request(params) { +proto.request = async function (params) { + if (this.options.retryMax) { + this.request = retry(request.bind(this), this.options.retryMax, { + errorHandler: (err) => { + const _errHandle = (_err) => { + const statusErr = [-1, -2].includes(_err.status); + const requestErrorRetryHandle = this.options.requestErrorRetryHandle || (() => true); + return statusErr && requestErrorRetryHandle(_err); + }; + if (_errHandle(err)) return true; + return false; + } + }); + } else { + this.request = request.bind(this); + } + + return await this.request(params); +}; + +async function request(params) { const reqParams = createRequest.call(this, params); if (!this.options.useFetch) { diff --git a/lib/client.js b/lib/client.js index 4da99e141..44503209b 100644 --- a/lib/client.js +++ b/lib/client.js @@ -16,6 +16,7 @@ const { createRequest } = require('./common/utils/createRequest'); const { encoder } = require('./common/utils/encoder'); const { getReqUrl } = require('./common/client/getReqUrl'); const { setSTSToken } = require('./common/utils/setSTSToken'); +const { retry } = require('./common/utils/retry'); const globalHttpAgent = new AgentKeepalive(); const globalHttpsAgent = new HttpsAgentKeepalive(); @@ -165,7 +166,28 @@ proto.authorization = function authorization(method, resource, subres, headers) * @api private */ -proto.request = async function request(params) { +proto.request = async function (params) { + const isAvailableStream = params.stream ? params.stream.readable : true; + if (this.options.retryMax && isAvailableStream) { + this.request = retry(request.bind(this), this.options.retryMax, { + errorHandler: (err) => { + const _errHandle = (_err) => { + const statusErr = [-1, -2].includes(_err.status); + const requestErrorRetryHandle = this.options.requestErrorRetryHandle || (() => true); + return statusErr && requestErrorRetryHandle(_err); + }; + if (_errHandle(err)) return true; + return false; + } + }); + } else { + this.request = request.bind(this); + } + + return await this.request(params); +}; + +async function request(params) { const reqParams = createRequest.call(this, params); let result; let reqErr; diff --git a/lib/common/utils/retry.d.ts b/lib/common/utils/retry.d.ts new file mode 100644 index 000000000..27eff2577 --- /dev/null +++ b/lib/common/utils/retry.d.ts @@ -0,0 +1 @@ +export declare function retry(this: any, func: Function, retryMax: number, config?: any): (...arg: any[]) => Promise; diff --git a/lib/common/utils/retry.js b/lib/common/utils/retry.js new file mode 100644 index 000000000..5d259613a --- /dev/null +++ b/lib/common/utils/retry.js @@ -0,0 +1,30 @@ +"use strict"; +Object.defineProperty(exports, "__esModule", { value: true }); +exports.retry = void 0; +function retry(func, retryMax, config = {}) { + let retryNum = 0; + const { retryDelay = 500, errorHandler = () => true } = config; + const funcR = (...arg) => { + return new Promise((resolve, reject) => { + func(...arg) + .then(result => { + retryNum = 0; + resolve(result); + }) + .catch(err => { + if (retryNum < retryMax && errorHandler(err)) { + retryNum++; + setTimeout(() => { + resolve(funcR(...arg)); + }, retryDelay); + } + else { + retryNum = 0; + reject(err); + } + }); + }); + }; + return funcR; +} +exports.retry = retry; diff --git a/lib/common/utils/retry.ts b/lib/common/utils/retry.ts new file mode 100644 index 000000000..844806c5c --- /dev/null +++ b/lib/common/utils/retry.ts @@ -0,0 +1,26 @@ +export function retry(this: any, func: Function, retryMax: number, config: any = {}) { + let retryNum = 0; + const { retryDelay = 500, errorHandler = () => true } = config; + const funcR = (...arg) => { + return new Promise((resolve, reject) => { + func(...arg) + .then(result => { + retryNum = 0; + resolve(result); + }) + .catch(err => { + if (retryNum < retryMax && errorHandler(err)) { + retryNum++; + setTimeout(() => { + resolve(funcR(...arg)); + }, retryDelay); + } else { + retryNum = 0; + reject(err); + } + }); + }); + }; + + return funcR; +} diff --git a/package.json b/package.json index 43cd427ac..f6e58a8d7 100644 --- a/package.json +++ b/package.json @@ -21,8 +21,8 @@ }, "scripts": { "build-change-log": "standard-version", - "test": "mocha -t 60000 -r thunk-mocha -r should test/node/*.test.js", - "test-cov": "nyc --reporter=lcov node_modules/.bin/_mocha -t 60000 -r thunk-mocha -r should test/node/*.test.js", + "test": "mocha -t 60000 -r thunk-mocha -r should test/node/*.test.js test/node/**/*.test.js", + "test-cov": "nyc --reporter=lcov node_modules/.bin/_mocha -t 60000 -r thunk-mocha -r should test/node/*.test.js test/node/**/*.test.js", "jshint": "jshint .", "autod": "autod", "build-test": "MINIFY=1 node browser-build.js > test/browser/build/aliyun-oss-sdk.min.js && node task/browser-test-build.js > test/browser/build/tests.js", diff --git a/test/browser/browser.test.js b/test/browser/browser.test.js index 46fa744bd..cdc8be474 100644 --- a/test/browser/browser.test.js +++ b/test/browser/browser.test.js @@ -1555,7 +1555,7 @@ describe('browser', () => { const store = oss(ossConfig); const name = `${prefix}put/skew_date`; const body = Buffer.from('body'); - const requestSpy = sinon.spy(store, 'request'); + const requestSpy = sinon.spy(store.urllib, 'request'); const requestErrorSpy = sinon.spy(store, 'requestError'); timemachine.config({ @@ -1575,7 +1575,8 @@ describe('browser', () => { const resultDel = await store.delete(name); assert.equal(resultDel.res.status, 204); - + store.urllib.request.restore(); + store.requestError.restore(); timemachine.reset(); }); @@ -1583,7 +1584,7 @@ describe('browser', () => { it('date is skew, put file will retry', async () => { const store = oss(ossConfig); const name = `${prefix}put/skew_date_file`; - const requestSpy = sinon.spy(store, 'request'); + const requestSpy = sinon.spy(store.urllib, 'request'); const requestErrorSpy = sinon.spy(store, 'requestError'); const fileContent = Array(1024 * 1024).fill('a').join(''); const file = new File([fileContent], 'skew_date_file'); @@ -1605,7 +1606,8 @@ describe('browser', () => { const resultDel = await store.delete(name); assert.equal(resultDel.res.status, 204); - + store.urllib.request.restore(); + store.requestError.restore(); timemachine.reset(); }); }); @@ -1739,4 +1741,76 @@ describe('browser', () => { assert.equal(info.meta.b, latin1_content); }); }); + + describe('test/retry.test.js', () => { + let store; + const RETRY_MAX = 3; + let testRetryCount = 0; + let autoRestoreWhenRETRY_LIMIE = true; + + let ORIGIN_REQUEST; + const mock = () => { + store.urllib.request = () => { + const e = new Error('NetError'); + e.status = -1; + throw e; + }; + }; + const restore = () => { + store.urllib.request = ORIGIN_REQUEST; + }; + + before(async () => { + const ossConfigz = { + region: stsConfig.region, + accessKeyId: stsConfig.Credentials.AccessKeyId, + accessKeySecret: stsConfig.Credentials.AccessKeySecret, + stsToken: stsConfig.Credentials.SecurityToken, + bucket: stsConfig.bucket, + retryMax: RETRY_MAX, + requestErrorRetryHandle: () => { + testRetryCount++; + if (testRetryCount === RETRY_MAX && autoRestoreWhenRETRY_LIMIE) { + restore(); + } + return true; + } + }; + store = oss(ossConfigz); + ORIGIN_REQUEST = store.urllib.request; + }); + beforeEach(() => { + testRetryCount = 0; + autoRestoreWhenRETRY_LIMIE = true; + mock(); + }); + afterEach(() => { + restore(); + }); + + it('set retryMax to test request auto retry when networkError or timeout', async () => { + const res = await store.list(); + assert.strictEqual(res.res.status, 200); + assert.strictEqual(testRetryCount, RETRY_MAX); + }); + + it('should throw when retry count bigger than options retryMax', async () => { + autoRestoreWhenRETRY_LIMIE = false; + try { + await store.list(); + assert(false, 'should throw error'); + } catch (error) { + assert(error.status === -1); + } + }); + + it('should succeed when put with filename', async () => { + const name = `ali-oss-test-retry-file-${Date.now()}`; + const res = await store.put(name, new File([1, 2, 3, 4, 5, 6, 7], name)); + assert.strictEqual(res.res.status, 200); + assert.strictEqual(testRetryCount, RETRY_MAX); + const onlineFile = await store.get(name); + assert.strictEqual(onlineFile.content.toString(), '1234567'); + }); + }); }); diff --git a/test/node/utils/retry.test.js b/test/node/utils/retry.test.js new file mode 100644 index 000000000..1e050b997 --- /dev/null +++ b/test/node/utils/retry.test.js @@ -0,0 +1,86 @@ +const assert = require('assert'); +const OSS = require('../../..'); +const config = require('../../config').oss; +const utils = require('../utils'); +const { md5 } = require('utility'); +const mm = require('mm'); +const fs = require('fs'); +const { after } = require('mocha'); + +describe('test/retry.test.js', () => { + let store; + const RETRY_MAX = 3; + let testRetryCount = 0; + let autoRestoreWhenRETRY_LIMIE = true; + const bucket = `ali-oss-test-bucket-${utils.prefix.replace(/[/.]/g, '-').replace(/-$/, '')}`; + before(async () => { + store = new OSS({ + ...config, + retryMax: RETRY_MAX, + requestErrorRetryHandle: () => { + testRetryCount++; + if (testRetryCount === RETRY_MAX && autoRestoreWhenRETRY_LIMIE) { + mm.restore(); + } + return true; + } + }); + const result = await store.putBucket(bucket); + assert.strictEqual(result.bucket, bucket); + assert.strictEqual(result.res.status, 200); + store.useBucket(bucket); + }); + beforeEach(() => { + testRetryCount = 0; + autoRestoreWhenRETRY_LIMIE = true; + mm.error(store.urllib, 'request', { + status: -1, // timeout + headers: {}, + }); + }); + afterEach(() => { + mm.restore(); + }); + after(async () => { + await utils.cleanBucket(store, bucket); + }); + + it('set retryMax to test request auto retry when networkError or timeout', async () => { + const res = await store.listBuckets(); + assert.strictEqual(res.res.status, 200); + assert.strictEqual(testRetryCount, RETRY_MAX); + }); + + it('should throw when retry count bigger than options retryMax', async () => { + autoRestoreWhenRETRY_LIMIE = false; + try { + await store.listBuckets(); + assert(false, 'should throw error'); + } catch (error) { + assert(error.status === -1); + } + }); + + it('should succeed when put with filename', async () => { + const name = `ali-oss-test-retry-file-${Date.now()}`; + const fileName = await utils.createTempFile(name, 1 * 1024); + const res = await store.put(name, fileName); + assert.strictEqual(res.res.status, 200); + assert.strictEqual(testRetryCount, RETRY_MAX); + const onlineFile = await store.get(name); + assert.strictEqual(md5(fs.readFileSync(fileName)), md5(onlineFile.content)); + }); + + it('should fail when putStream', async () => { + autoRestoreWhenRETRY_LIMIE = false; + const name = `ali-oss-test-retry-file-${Date.now()}`; + const fileName = await utils.createTempFile(name, 1 * 1024); + try { + await store.putStream(name, fs.createReadStream(fileName)); + assert(false, 'should not reach here'); + } catch (e) { + assert.strictEqual(e.status, -1); + } + }); + +});