From 94d7f5e456921b4f54f1289336e249b13e79e399 Mon Sep 17 00:00:00 2001 From: killagu Date: Tue, 28 Jun 2022 14:35:19 +0800 Subject: [PATCH 1/5] fix: ignore request error if request is done --- lib/urllib.js | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/urllib.js b/lib/urllib.js index 4d429515..129e145f 100644 --- a/lib/urllib.js +++ b/lib/urllib.js @@ -1161,12 +1161,13 @@ function requestWithCallback(url, args, callback) { }); } - var isRequestError = false; + var isRequestDone = false; function handleRequestError(err) { - if (isRequestError || !err) { + console.log('req error: ', err); + if (isRequestDone || !err) { return; } - isRequestError = true; + isRequestDone = true; if (err.name === 'Error') { err.name = connected ? 'ResponseError' : 'RequestError'; @@ -1178,7 +1179,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 From fe76c340bd80683d2cecf853b813f42b78a4ada1 Mon Sep 17 00:00:00 2001 From: killagu Date: Tue, 28 Jun 2022 14:44:30 +0800 Subject: [PATCH 2/5] f --- lib/urllib.js | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/urllib.js b/lib/urllib.js index 129e145f..21008f43 100644 --- a/lib/urllib.js +++ b/lib/urllib.js @@ -1163,7 +1163,6 @@ function requestWithCallback(url, args, callback) { var isRequestDone = false; function handleRequestError(err) { - console.log('req error: ', err); if (isRequestDone || !err) { return; } From 2ffee7f5563a7492e6fc3e76e9a3f68dcd5f3731 Mon Sep 17 00:00:00 2001 From: killagu Date: Tue, 5 Jul 2022 10:24:13 +0800 Subject: [PATCH 3/5] ci: add request error after request finish case --- .github/workflows/nodejs.yml | 2 +- test/urllib.test.js | 17 +++++++++++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/.github/workflows/nodejs.yml b/.github/workflows/nodejs.yml index 61456bb4..f973cea7 100644 --- a/.github/workflows/nodejs.yml +++ b/.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 diff --git a/test/urllib.test.js b/test/urllib.test.js index e545f51f..e4a0a196 100644 --- a/test/urllib.test.js +++ b/test/urllib.test.js @@ -980,6 +980,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('finish', () => { + req.emit('error', new Error('mock error')); + }); + }); }); describe('support stream', function () { From 400488483be3c1d53e6db583df73793e7d25bd8f Mon Sep 17 00:00:00 2001 From: killagu Date: Tue, 5 Jul 2022 11:38:35 +0800 Subject: [PATCH 4/5] f --- lib/urllib.js | 26 ++++++++++++++++++-------- test/proxy.test.js | 9 +++------ test/urllib.test.js | 10 ++++------ 3 files changed, 25 insertions(+), 20 deletions(-) diff --git a/lib/urllib.js b/lib/urllib.js index 21008f43..858496cc 100644 --- a/lib/urllib.js +++ b/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); }); } @@ -1163,7 +1168,12 @@ function requestWithCallback(url, args, callback) { var isRequestDone = false; function handleRequestError(err) { - if (isRequestDone || !err) { + if (!err) { + return; + } + // 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; diff --git a/test/proxy.test.js b/test/proxy.test.js index d8b9af14..5b061782 100644 --- a/test/proxy.test.js +++ b/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(); }); }); diff --git a/test/urllib.test.js b/test/urllib.test.js index e4a0a196..5337ee67 100644 --- a/test/urllib.test.js +++ b/test/urllib.test.js @@ -411,11 +411,9 @@ 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); + assert(err.code === 'ECONNRESET'); + assert(err.data); + assert(data); done(); }); }); @@ -993,7 +991,7 @@ describe('test/urllib.test.js', function () { done(); }); - req.on('finish', () => { + req.on('response', () => { req.emit('error', new Error('mock error')); }); }); From c1fb62544e05a3677195f83129bd5d4aa7db5952 Mon Sep 17 00:00:00 2001 From: killagu Date: Tue, 5 Jul 2022 12:04:06 +0800 Subject: [PATCH 5/5] f --- test/urllib.test.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/urllib.test.js b/test/urllib.test.js index 5337ee67..5e214c59 100644 --- a/test/urllib.test.js +++ b/test/urllib.test.js @@ -411,8 +411,7 @@ 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.code === 'ECONNRESET'); - assert(err.data); + err.code && assert(err.code === 'ECONNRESET'); assert(data); done(); });