From 004bec42a1f548c78e9f35677f2287e637992be2 Mon Sep 17 00:00:00 2001 From: Robert Nagy Date: Thu, 29 Apr 2021 21:21:44 +0200 Subject: [PATCH 1/9] fix: end message on connection close If response is not keep alive and socket ends then consider it as end of message. --- lib/client.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/lib/client.js b/lib/client.js index 8fcbd0f1f96..914e8dcc8d7 100644 --- a/lib/client.js +++ b/lib/client.js @@ -932,6 +932,13 @@ function onSocketError (err) { } function onSocketEnd () { + const { [kParser]: parser } = this + + if (this.statusCode && !this.shouldKeepAlive) { + // Response does not contain content-length nor chunked encoding. + parser.onMessageComplete() + } + util.destroy(this, new SocketError('other side closed')) } From 9d07904c24ff576a325946463e0a5566fb4806c0 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 10:25:26 +0200 Subject: [PATCH 2/9] Added test and fixed the bug --- lib/client.js | 4 +++- test/client-request.js | 40 ++++++++++++++++++++++++++++++++++++++++ test/content-length.js | 4 +++- 3 files changed, 46 insertions(+), 2 deletions(-) diff --git a/lib/client.js b/lib/client.js index 914e8dcc8d7..e26c2de0406 100644 --- a/lib/client.js +++ b/lib/client.js @@ -934,9 +934,11 @@ function onSocketError (err) { function onSocketEnd () { const { [kParser]: parser } = this - if (this.statusCode && !this.shouldKeepAlive) { + if (parser.statusCode && !parser.shouldKeepAlive) { // Response does not contain content-length nor chunked encoding. parser.onMessageComplete() + util.destroy() + return } util.destroy(this, new SocketError('other side closed')) diff --git a/test/client-request.js b/test/client-request.js index 8bcea5bd8fa..828f3d51d25 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -6,6 +6,8 @@ const { createServer } = require('http') const EE = require('events') const { kConnect } = require('../lib/core/symbols') const { Readable } = require('stream') +const net = require('net') +const { promisify } = require('util') test('request abort before headers', (t) => { t.plan(6) @@ -97,3 +99,41 @@ test('trailers', (t) => { }) }) }) + +test('destroy socket abruptly', async (t) => { + t.plan(2) + + const server = net.createServer((socket) => { + const lines = [ + 'HTTP/1.1 200 OK', + 'Date: Sat, 09 Oct 2010 14:28:02 GMT', + 'Connection: close', + '', + 'the body' + ] + socket.end(lines.join('\r\n')) + socket.destroy() + }) + t.teardown(server.close.bind(server)) + + await promisify(server.listen.bind(server))(0) + const client = new Client(`http://localhost:${server.address().port}`) + t.teardown(client.close.bind(client)) + + const { statusCode, body } = await client.request({ + path: '/', + method: 'GET' + }) + + t.equal(statusCode, 200) + + body.setEncoding('utf8') + + let actual = '' + + for await (const chunk of body) { + actual += chunk + } + + t.equal(actual, 'the body') +}) diff --git a/test/content-length.js b/test/content-length.js index 3329765d2e2..977b7121c12 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -231,7 +231,9 @@ test('request streaming no body data when content-length=0', (t) => { }) }) -test('response invalid content length with close', (t) => { +// This test is skipped because we are not currently tracking how +// many bytes we have received. +test('response invalid content length with close', { skip: true }, (t) => { t.plan(3) const server = createServer((req, res) => { From 658d16d2ec657c18d841b922840ec59929bd2fc4 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 10:35:38 +0200 Subject: [PATCH 3/9] added setImmediate --- test/client-request.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/test/client-request.js b/test/client-request.js index 828f3d51d25..8f0154b8e20 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -112,7 +112,12 @@ test('destroy socket abruptly', async (t) => { 'the body' ] socket.end(lines.join('\r\n')) - socket.destroy() + + // Unfortunately calling destroy synchronously might get us flaky results, + // therefore we delay it to the next event loop run. + setImmediate(() => { + socket.destroy() + }) }) t.teardown(server.close.bind(server)) From 318b9e38cb3fc8158008a6ecc576e2bdc2e349b0 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 11:38:18 +0200 Subject: [PATCH 4/9] use llhttp_finish --- lib/client.js | 23 +++++++++++++++++++---- test/client-request.js | 4 +--- test/client.js | 2 ++ test/content-length.js | 4 +--- 4 files changed, 23 insertions(+), 10 deletions(-) diff --git a/lib/client.js b/lib/client.js index e26c2de0406..5463718962a 100644 --- a/lib/client.js +++ b/lib/client.js @@ -794,9 +794,9 @@ class Parser { } onMessageComplete () { - const { client, socket, statusCode, upgrade, trailers, headers: rawTrailers } = this + const { client, socket, statusCode, upgrade, trailers, headers: rawTrailers, shouldKeepAlive } = this - if (socket.destroyed) { + if (socket.destroyed && (!statusCode || shouldKeepAlive)) { return -1 } @@ -863,6 +863,10 @@ class Parser { resume(client) } } + + finish () { + llhttp.exports.llhttp_finish(this.ptr) + } } function onParserTimeout (parser) { @@ -901,6 +905,16 @@ function onSocketConnect () { } function onSocketError (err) { + const { [kParser]: parser } = this + if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { + // Response does not contain content-length nor chunked encoding. + // We treat all incoming data so for as a valid response. + currentParser = parser + parser.finish() + currentParser = null + return + } + const { [kClient]: client } = this this[kError] = err @@ -936,8 +950,9 @@ function onSocketEnd () { if (parser.statusCode && !parser.shouldKeepAlive) { // Response does not contain content-length nor chunked encoding. - parser.onMessageComplete() - util.destroy() + currentParser = parser + parser.finish() + currentParser = null return } diff --git a/test/client-request.js b/test/client-request.js index 8f0154b8e20..ec079aa952e 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -115,9 +115,7 @@ test('destroy socket abruptly', async (t) => { // Unfortunately calling destroy synchronously might get us flaky results, // therefore we delay it to the next event loop run. - setImmediate(() => { - socket.destroy() - }) + socket.destroy() }) t.teardown(server.close.bind(server)) diff --git a/test/client.js b/test/client.js index 50ebfc92ef2..fc64643a0e4 100644 --- a/test/client.js +++ b/test/client.js @@ -1004,6 +1004,8 @@ test('connected', (t) => { t.plan(7) const server = createServer((req, res) => { + // needed so that disconnect is emitted + res.setHeader('connection', 'close') req.pipe(res) }) t.teardown(server.close.bind(server)) diff --git a/test/content-length.js b/test/content-length.js index 977b7121c12..3329765d2e2 100644 --- a/test/content-length.js +++ b/test/content-length.js @@ -231,9 +231,7 @@ test('request streaming no body data when content-length=0', (t) => { }) }) -// This test is skipped because we are not currently tracking how -// many bytes we have received. -test('response invalid content length with close', { skip: true }, (t) => { +test('response invalid content length with close', (t) => { t.plan(3) const server = createServer((req, res) => { From bc17a2ec1dadd693984056f652a353cd0293a3ce Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 11:46:41 +0200 Subject: [PATCH 5/9] Added test for keep-alive --- test/client-request.js | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/test/client-request.js b/test/client-request.js index ec079aa952e..cbacf5063a1 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -140,3 +140,44 @@ test('destroy socket abruptly', async (t) => { t.equal(actual, 'the body') }) + +test('destroy socket abruptly with keep-alive', async (t) => { + t.plan(2) + + const server = net.createServer((socket) => { + const lines = [ + 'HTTP/1.1 200 OK', + 'Date: Sat, 09 Oct 2010 14:28:02 GMT', + 'Connection: keep-alive', + '', + 'the body' + ] + socket.end(lines.join('\r\n')) + + // Unfortunately calling destroy synchronously might get us flaky results, + // therefore we delay it to the next event loop run. + socket.destroy() + }) + t.teardown(server.close.bind(server)) + + await promisify(server.listen.bind(server))(0) + const client = new Client(`http://localhost:${server.address().port}`) + t.teardown(client.close.bind(client)) + + const { statusCode, body } = await client.request({ + path: '/', + method: 'GET' + }) + + t.equal(statusCode, 200) + + body.setEncoding('utf8') + + let actual = '' + + for await (const chunk of body) { + actual += chunk + } + + t.equal(actual, 'the body') +}) From c4c9c91450c1d15b2440b385a40495b7f6d5df9f Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 11:51:33 +0200 Subject: [PATCH 6/9] Added a comment about Mac OS. --- lib/client.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/client.js b/lib/client.js index 5463718962a..1cda104765f 100644 --- a/lib/client.js +++ b/lib/client.js @@ -906,6 +906,9 @@ function onSocketConnect () { function onSocketError (err) { const { [kParser]: parser } = this + + // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded + // to the user. if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { // Response does not contain content-length nor chunked encoding. // We treat all incoming data so for as a valid response. From c02c205f9882854c8601c397dd5bf438bfe9993e Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 12:00:48 +0200 Subject: [PATCH 7/9] Use setImmediate --- test/client-request.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/client-request.js b/test/client-request.js index cbacf5063a1..dccb4e62b6a 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -115,7 +115,7 @@ test('destroy socket abruptly', async (t) => { // Unfortunately calling destroy synchronously might get us flaky results, // therefore we delay it to the next event loop run. - socket.destroy() + setImmediate(socket.destroy.bind(socket)) }) t.teardown(server.close.bind(server)) @@ -156,7 +156,7 @@ test('destroy socket abruptly with keep-alive', async (t) => { // Unfortunately calling destroy synchronously might get us flaky results, // therefore we delay it to the next event loop run. - socket.destroy() + setImmediate(socket.destroy.bind(socket)) }) t.teardown(server.close.bind(server)) From 25fd2aefa70fa577604c1ffd93741d83311cf1ce Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 13:35:35 +0200 Subject: [PATCH 8/9] fixup --- lib/client.js | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/lib/client.js b/lib/client.js index 1cda104765f..e7af566335b 100644 --- a/lib/client.js +++ b/lib/client.js @@ -541,6 +541,12 @@ class Parser { } } + finish () { + currentParser = this + llhttp.exports.llhttp_finish(this.ptr) + currentParser = null + } + destroy () { assert(this.ptr != null) assert(currentParser == null) @@ -863,10 +869,6 @@ class Parser { resume(client) } } - - finish () { - llhttp.exports.llhttp_finish(this.ptr) - } } function onParserTimeout (parser) { @@ -910,11 +912,8 @@ function onSocketError (err) { // On Mac OS, we get an ECONNRESET even if there is a full body to be forwarded // to the user. if (err.code === 'ECONNRESET' && parser.statusCode && !parser.shouldKeepAlive) { - // Response does not contain content-length nor chunked encoding. // We treat all incoming data so for as a valid response. - currentParser = parser parser.finish() - currentParser = null return } @@ -952,10 +951,8 @@ function onSocketEnd () { const { [kParser]: parser } = this if (parser.statusCode && !parser.shouldKeepAlive) { - // Response does not contain content-length nor chunked encoding. - currentParser = parser + // We treat all incoming data so for as a valid response. parser.finish() - currentParser = null return } From 7d938df176b8b8191d36b460911824311fa890ec Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Mon, 3 May 2021 16:17:47 +0200 Subject: [PATCH 9/9] create error test for keep-alive --- test/client-request.js | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/test/client-request.js b/test/client-request.js index dccb4e62b6a..05a1707b707 100644 --- a/test/client-request.js +++ b/test/client-request.js @@ -149,6 +149,7 @@ test('destroy socket abruptly with keep-alive', async (t) => { 'HTTP/1.1 200 OK', 'Date: Sat, 09 Oct 2010 14:28:02 GMT', 'Connection: keep-alive', + 'Content-Length: 42', '', 'the body' ] @@ -173,11 +174,14 @@ test('destroy socket abruptly with keep-alive', async (t) => { body.setEncoding('utf8') - let actual = '' - - for await (const chunk of body) { - actual += chunk + try { + /* eslint-disable */ + for await (const _ of body) { + // empty on purpose + } + /* eslint-enable */ + t.fail('no error') + } catch (err) { + t.pass('error happened') } - - t.equal(actual, 'the body') })