fix: ignore request error if request is done (#384)

* ci: add request error after request finish case

Author: killagu

.github/workflows/nodejs.yml

@@ -35,7 +35,7 @@ jobs:
         node-version: ${{ matrix.node-version }}
 
     - name: Install Dependencies
-      run: npm i -g npminstall && npminstall
+      run: npm i -g npminstall@latest-5 && npminstall
 
     - name: Continuous Integration
       run: npm run ci

lib/urllib.js

@@ -728,6 +728,13 @@ function requestWithCallback(url, args, callback) {
   }
 
   function decodeContent(res, body, cb) {
+    if (responseAborted) {
+      // err = new Error('Remote socket was terminated before `response.end()` was called');
+      // err.name = 'RemoteSocketClosedError';
+      debug('Request#%d %s: Remote socket was terminated before `response.end()` was called', reqId, url);
+      var err = responseError || new Error('Remote socket was terminated before `response.end()` was called');
+      return cb(err);
+    }
     var encoding = res.headers['content-encoding'];
     if (body.length === 0 || !encoding) {
       return cb(null, body, encoding);
@@ -758,7 +765,10 @@ function requestWithCallback(url, args, callback) {
 
   args.requestUrls.push(parsedUrl.href);
 
+  var hasResponse = false;
+  var responseError;
   function onResponse(res) {
+    hasResponse = true;
     socketHandledResponses = res.socket[SOCKET_RESPONSE_COUNT] = (res.socket[SOCKET_RESPONSE_COUNT] || 0) + 1;
     if (timing) {
       timing.waiting = Date.now() - requestStartTime;
@@ -781,7 +791,8 @@ function requestWithCallback(url, args, callback) {
       return done(null, null, res);
     }
 
-    res.on('error', function () {
+    res.on('error', function (err) {
+      responseError = err;
       debug('Request#%d %s: `res error` event emit, total size %d, socket handled %s requests and %s responses',
         reqId, url, responseSize, socketHandledRequests, socketHandledResponses);
     });
@@ -939,12 +950,6 @@ function requestWithCallback(url, args, callback) {
           }
         }
 
-        if (responseAborted) {
-          // err = new Error('Remote socket was terminated before `response.end()` was called');
-          // err.name = 'RemoteSocketClosedError';
-          debug('Request#%d %s: Remote socket was terminated before `response.end()` was called', reqId, url);
-        }
-
         done(err, data, res);
       });
     }
@@ -1161,12 +1166,17 @@ function requestWithCallback(url, args, callback) {
     });
   }
 
-  var isRequestError = false;
+  var isRequestDone = false;
   function handleRequestError(err) {
-    if (isRequestError || !err) {
+    if (!err) {
       return;
     }
-    isRequestError = true;
+    // only ignore request error if response has been received
+    // if response has not received, socket error will emit on req
+    if (isRequestDone && hasResponse) {
+      return;
+    }
+    isRequestDone = true;
 
     if (err.name === 'Error') {
       err.name = connected ? 'ResponseError' : 'RequestError';
@@ -1178,7 +1188,9 @@ function requestWithCallback(url, args, callback) {
     debug('Request#%d pump args.stream to req', reqId);
     pump(args.stream, req, handleRequestError);
   } else {
-    req.end(body);
+    req.end(body, function () {
+      isRequestDone = true;
+    });
   }
   // when stream already consumed, req's `finish` event is emitted and pump will ignore error after pipe finished
   // but if server response timeout later, we will abort the request and emit an error in req

test/proxy.test.js

@@ -6,7 +6,7 @@ var proxy = require('./fixtures/reverse-proxy');
 var isNode010 = /^v0\.10\.\d+$/.test(process.version);
 var isNode012 = /^v0\.12\.\d+$/.test(process.version);
 
-var testUrl = process.env.CI ? 'https://registry.npmjs.com' : 'https://registry.npm.taobao.org';
+var testUrl = process.env.CI ? 'https://registry.npmjs.com' : 'https://registry.npmmirror.com';
 
 if (!isNode010 && !isNode012) {
   describe('test/proxy.test.js', function() {
@@ -25,15 +25,12 @@ if (!isNode010 && !isNode012) {
     });
 
     it('should proxy http work', function(done) {
-      urllib.request('http://registry.npm.taobao.org/pedding/1.0.0', {
-        dataType: 'json',
+      urllib.request('http://registry.npmmirror.com/pedding/1.0.0', {
         enableProxy: true,
         proxy: proxyUrl,
       }, function(err, data, res) {
         assert(!err);
-        console.log(res.headers);
-        assert(data.name === 'pedding');
-        assert(res.status === 200);
+        assert(res.status === 301);
         done();
       });
     });

test/urllib.test.js

@@ -411,11 +411,8 @@ describe('test/urllib.test.js', function () {
     it('should handle server socket end("balabal") will error', function (done) {
       urllib.request(host + '/socket.end.error', function (err, data) {
         assert(err);
-        assert(err.name === 'ResponseError');
-        err.code && assert(err.code === 'HPE_INVALID_CHUNK_SIZE');
-        assert(/Parse Error.*GET http:\/\/127\.0\.0\.1:/.test(err.message) >= 0);
-        assert(err.bytesParsed === 2);
-        assert(!data);
+        err.code && assert(err.code === 'ECONNRESET');
+        assert(data);
         done();
       });
     });
@@ -980,6 +977,23 @@ describe('test/urllib.test.js', function () {
         });
       });
     });
+
+    it('should not emit request error if request is done', function (done) {
+      var req = urllib.request(host + '/stream', {
+        method: 'post',
+        data: {foo: 'bar'},
+        ctx: {
+          foo: 'stream request'
+        }
+      }, function (err) {
+        assert(!err);
+        done();
+      });
+
+      req.on('response', () => {
+        req.emit('error', new Error('mock error'));
+      });
+    });
   });
 
   describe('support stream', function () {