fix: 🐛 should trigger response event when follow redirect (#361)

Author: denghongcai

lib/urllib.js

@@ -520,6 +520,7 @@ function requestWithCallback(url, args, callback) {
       }
       return;
     }
+
     var cb = callback;
     callback = null;
     var headers = {};
@@ -529,49 +530,16 @@ function requestWithCallback(url, args, callback) {
       headers = res.headers;
     }
 
-    // handle digest auth
-    if (statusCode === 401 && headers['www-authenticate']
-        && !options.headers.authorization && args.digestAuth) {
-      var authenticate = headers['www-authenticate'];
-      if (authenticate.indexOf('Digest ') >= 0) {
-        debug('Request#%d %s: got digest auth header WWW-Authenticate: %s', reqId, url, authenticate);
-        options.headers.authorization = digestAuthHeader(options.method, options.path, authenticate, args.digestAuth);
-        debug('Request#%d %s: auth with digest header: %s', reqId, url, options.headers.authorization);
-        if (res.headers['set-cookie']) {
-          options.headers.cookie = res.headers['set-cookie'].join(';');
-        }
-        args.headers = options.headers;
-        return exports.requestWithCallback(url, args, cb);
-      }
+    if (handleDigestAuth(res, cb)) {
+      return;
     }
 
-    var requestUseTime = Date.now() - requestStartTime;
-    if (timing) {
-      timing.contentDownload = requestUseTime;
-    }
+    var response = createCallbackResponse(data, res);
 
     debug('[%sms] done, %s bytes HTTP %s %s %s %s, keepAliveSocket: %s, timing: %j, socketHandledRequests: %s, socketHandledResponses: %s',
-      requestUseTime, responseSize, statusCode, options.method, options.host, options.path,
+      response.requestUseTime, responseSize, statusCode, options.method, options.host, options.path,
       keepAliveSocket, timing, socketHandledRequests, socketHandledResponses);
 
-    var response = {
-      status: statusCode,
-      statusCode: statusCode,
-      statusMessage: statusMessage,
-      headers: headers,
-      size: responseSize,
-      aborted: responseAborted,
-      rt: requestUseTime,
-      keepAliveSocket: keepAliveSocket,
-      data: data,
-      requestUrls: args.requestUrls,
-      timing: timing,
-      remoteAddress: remoteAddress,
-      remotePort: remotePort,
-      socketHandledRequests: socketHandledRequests,
-      socketHandledResponses: socketHandledResponses,
-    };
-
     if (err) {
       var agentStatus = '';
       if (agent && typeof agent.getCurrentStatus === 'function') {
@@ -620,6 +588,45 @@ function requestWithCallback(url, args, callback) {
 
     cb(err, data, args.streaming ? res : response);
 
+    emitResponseEvent(err, response);
+  }
+
+  function createAndEmitResponseEvent(data, res) {
+    var response = createCallbackResponse(data, res);
+    emitResponseEvent(null, response);
+  }
+
+  function createCallbackResponse(data, res) {
+    var requestUseTime = Date.now() - requestStartTime;
+    if (timing) {
+      timing.contentDownload = requestUseTime;
+    }
+
+    var headers = {};
+    if (res && res.headers) {
+      headers = res.headers;
+    }
+
+    return {
+      status: statusCode,
+      statusCode: statusCode,
+      statusMessage: statusMessage,
+      headers: headers,
+      size: responseSize,
+      aborted: responseAborted,
+      rt: requestUseTime,
+      keepAliveSocket: keepAliveSocket,
+      data: data,
+      requestUrls: args.requestUrls,
+      timing: timing,
+      remoteAddress: remoteAddress,
+      remotePort: remotePort,
+      socketHandledRequests: socketHandledRequests,
+      socketHandledResponses: socketHandledResponses,
+    };
+  }
+
+  function emitResponseEvent(err, response) {
     if (args.emitter) {
       // keep to use the same reqMeta object on request event before
       reqMeta.url = parsedUrl.href;
@@ -637,6 +644,30 @@ function requestWithCallback(url, args, callback) {
     }
   }
 
+  function handleDigestAuth(res, cb) {
+    var headers = {};
+    if (res && res.headers) {
+      headers = res.headers;
+    }
+    // handle digest auth
+    if (statusCode === 401 && headers['www-authenticate']
+        && !options.headers.authorization && args.digestAuth) {
+      var authenticate = headers['www-authenticate'];
+      if (authenticate.indexOf('Digest ') >= 0) {
+        debug('Request#%d %s: got digest auth header WWW-Authenticate: %s', reqId, url, authenticate);
+        options.headers.authorization = digestAuthHeader(options.method, options.path, authenticate, args.digestAuth);
+        debug('Request#%d %s: auth with digest header: %s', reqId, url, options.headers.authorization);
+        if (res.headers['set-cookie']) {
+          options.headers.cookie = res.headers['set-cookie'].join(';');
+        }
+        args.headers = options.headers;
+        exports.requestWithCallback(url, args, cb);
+        return true;
+      }
+    }
+    return false;
+  }
+
   function handleRedirect(res) {
     var err = null;
     if (args.followRedirect && statuses.redirect[res.statusCode]) {  // handle redirect
@@ -740,6 +771,7 @@ function requestWithCallback(url, args, callback) {
       var result = handleRedirect(res);
       if (result.redirect) {
         res.resume();
+        createAndEmitResponseEvent(null, res);
         return;
       }
       if (result.error) {
@@ -781,6 +813,7 @@ function requestWithCallback(url, args, callback) {
       var result = handleRedirect(res);
       if (result.redirect) {
         res.resume();
+        createAndEmitResponseEvent(null, res);
         return;
       }
       if (result.error) {
@@ -874,6 +907,7 @@ function requestWithCallback(url, args, callback) {
         return done(result.error, body, res);
       }
       if (result.redirect) {
+        createAndEmitResponseEvent(null, res);
         return;
       }
 

test/config.js

@@ -5,7 +5,7 @@ module.exports = process.env.CI ? {
   npmRegistry: 'https://registry.npmjs.com',
   npmHttpRegistry: 'http://registry.npmjs.com',
 } : {
-  npmWeb: 'https://developer.aliyun.com/mirror/npm',
+  npmWeb: 'https://www.npmjs.com',
   npmRegistry: 'https://registry.npm.taobao.org',
   npmHttpRegistry: 'http://registry.npm.taobao.org',
 };

test/files.test.js

@@ -70,7 +70,7 @@ describe('test/files.test.js', function() {
       assert(result.headers['content-type'].indexOf('multipart/form-data;') === 0);
       assert(res.status === 200);
       assert(result.files.file.filename === 'bufferfile0');
-      assert(result.files.file.mimetype === 'application/octet-stream');
+      assert(result.files.file.mimetype === 'text/plain');
       done();
     });
   });
@@ -90,7 +90,7 @@ describe('test/files.test.js', function() {
       assert(result.files.file1.filename === 'files.test.js');
       assert(result.files.file1.mimetype === 'application/javascript');
       assert(result.files.file2.filename === 'bufferfile2');
-      assert(result.files.file2.mimetype === 'application/octet-stream');
+      assert(result.files.file2.mimetype === 'text/plain');
       done();
     });
   });

test/proxy.test.js

@@ -9,7 +9,7 @@ var isNode012 = /^v0\.12\.\d+$/.test(process.version);
 var testUrl = process.env.CI ? 'https://registry.npmjs.com' : 'https://registry.npm.taobao.org';
 
 if (!isNode010 && !isNode012) {
-  describe.only('test/proxy.test.js', function() {
+  describe('test/proxy.test.js', function() {
     var port;
     var proxyUrl;
     before(function(done) {

test/urllib.test.js

@@ -1704,6 +1704,33 @@ describe('test/urllib.test.js', function () {
         done();
       });
     });
+
+    it('should trigger req/res in pair when followRedirect', function (done) {
+      done = pedding(5, done);
+      var redirected = false;
+      urllib.on('request', function (info) {
+        info.args.headers = info.args.headers || {};
+        info.args.headers['custom-header'] = 'custom-header';
+        done();
+      });
+      urllib.on('response', function (info) {
+        if (info.req.options.path === '/302-to-200') {
+          redirected = true;
+        }
+        assert(info.req.options.headers['custom-header'] === 'custom-header');
+        assert(redirected);
+        done();
+      });
+
+      urllib.request(host + '/302-to-200', {
+        followRedirect: true,
+        ctx: {
+          foo: 'error request'
+        }
+      }, function () {
+        done();
+      });
+    });
   });
 
   describe('charset support', function () {