feat: switch httpclient to httpclient2 for retry feature (#3626)

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<T, P> = Pick<T, Exclude<keyof T, P>>;
 
-  interface EggHttpClient extends HttpClient2 {}
+  interface EggHttpClient extends HttpClient<RequestOptions> {}
   interface EggHttpConstructor {
     new (app: Application): EggHttpClient;
   }
 
-  interface EggContextHttpClient extends HttpClient2 {}
+  interface EggContextHttpClient extends HttpClient<RequestOptions> {}
   interface EggContextHttpClientConstructor {
     new (ctx: Context): EggContextHttpClient;
   }

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

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,
+        });
+      });
+    };
   }
 }
 

test/fixtures/apps/httpclient-retry/package.json

@@ -0,0 +1,3 @@
+{
+  "name": "httpclient-retry"
+}

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);
+      });
+    });
+  });
 });

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}`);
         }