From 769f52b8d2f2e2322b0e5f45f1a68fb9ea451542 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Wed, 10 Jan 2018 18:48:49 +0100 Subject: [PATCH 01/11] Add failing tests for http2 --- test/compression.js | 39 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/test/compression.js b/test/compression.js index 013744cb..53793dc4 100644 --- a/test/compression.js +++ b/test/compression.js @@ -4,6 +4,7 @@ var Buffer = require('safe-buffer').Buffer var bytes = require('bytes') var crypto = require('crypto') var http = require('http') +var http2 = require('http2') var request = require('supertest') var zlib = require('zlib') @@ -301,6 +302,22 @@ describe('compression()', function () { .expect(200, done) }) + it('should work with http2 server', function (done) { + createHttp2Server({ threshold: 0 }, function (req, res) { + res.setHeader('Content-Type', 'text/plain') + res.end('hello, world') + }) + + var request = createHttp2Client().request({ + 'Accept-Encoding': 'gzip' + }) + request.on('response', (headers) => { + assert.equal(headers['content-encoding'], 'gzip') + done() + }) + request.end() + }) + describe('threshold', function () { it('should not compress responses below the threshold size', function (done) { var server = createServer({ threshold: '1kb' }, function (req, res) { @@ -661,6 +678,28 @@ function createServer (opts, fn) { }) } +function createHttp2Server (opts, fn) { + var _compression = compression(opts) + var server = http2.createServer(function (req, res) { + _compression(req, res, function (err) { + if (err) { + res.statusCode = err.status || 500 + res.end(err.message) + return + } + + fn(req, res) + }) + }) + server.listen(8443, '127.0.0.1') + return server +} + +function createHttp2Client () { + var client = http2.connect('http://127.0.0.1:8443') + return client +} + function shouldHaveBodyLength (length) { return function (res) { assert.equal(res.text.length, length, 'should have body length of ' + length) From 6e1ae362b5f8b4f3b1ac801d93cbf64e340d0a51 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Wed, 10 Jan 2018 18:50:03 +0100 Subject: [PATCH 02/11] Add support for http2 The change just removes the usage of undocumented http API and instead uses the proper writeHead method --- index.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/index.js b/index.js index f190c689..8557808e 100644 --- a/index.js +++ b/index.js @@ -81,7 +81,7 @@ function compression (options) { } if (!this._header) { - this._implicitHeader() + this.writeHead(this.statusCode) } return stream @@ -100,7 +100,7 @@ function compression (options) { length = chunkLength(chunk, encoding) } - this._implicitHeader() + this.writeHead(this.statusCode) } if (!stream) { From f4e4dfc516e0a90a9dd32cc4da3e14bbfc65b3a2 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Wed, 10 Jan 2018 19:09:56 +0100 Subject: [PATCH 03/11] Remove arrow function in tests --- test/compression.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/compression.js b/test/compression.js index 53793dc4..ea21ed8c 100644 --- a/test/compression.js +++ b/test/compression.js @@ -308,11 +308,13 @@ describe('compression()', function () { res.end('hello, world') }) - var request = createHttp2Client().request({ + var client = createHttp2Client() + var request = client.request({ 'Accept-Encoding': 'gzip' }) - request.on('response', (headers) => { + request.on('response', function (headers) { assert.equal(headers['content-encoding'], 'gzip') + client.destroy() done() }) request.end() From b00e4f67b83e4f19b006eb30ef5b1708faede853 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Wed, 10 Jan 2018 19:58:51 +0100 Subject: [PATCH 04/11] Conditionally run http2 tests if http2 is available --- test/compression.js | 41 +++++++++++++++++++++++++---------------- 1 file changed, 25 insertions(+), 16 deletions(-) diff --git a/test/compression.js b/test/compression.js index ea21ed8c..504f4281 100644 --- a/test/compression.js +++ b/test/compression.js @@ -4,10 +4,17 @@ var Buffer = require('safe-buffer').Buffer var bytes = require('bytes') var crypto = require('crypto') var http = require('http') -var http2 = require('http2') var request = require('supertest') var zlib = require('zlib') +try { + var http2 = require('http2') +} catch (err) { + if (err) { + console.log('http2 tests disabled.') + } +} + var compression = require('..') describe('compression()', function () { @@ -302,23 +309,25 @@ describe('compression()', function () { .expect(200, done) }) - it('should work with http2 server', function (done) { - createHttp2Server({ threshold: 0 }, function (req, res) { - res.setHeader('Content-Type', 'text/plain') - res.end('hello, world') - }) + if (http2) { + it('should work with http2 server', function (done) { + createHttp2Server({ threshold: 0 }, function (req, res) { + res.setHeader('Content-Type', 'text/plain') + res.end('hello, world') + }) - var client = createHttp2Client() - var request = client.request({ - 'Accept-Encoding': 'gzip' - }) - request.on('response', function (headers) { - assert.equal(headers['content-encoding'], 'gzip') - client.destroy() - done() + var client = createHttp2Client() + var request = client.request({ + 'Accept-Encoding': 'gzip' + }) + request.on('response', function (headers) { + assert.equal(headers['content-encoding'], 'gzip') + client.destroy() + done() + }) + request.end() }) - request.end() - }) + } describe('threshold', function () { it('should not compress responses below the threshold size', function (done) { From 655bbe9385ae29f9cac3ca630be227f646d7b73c Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 17:51:16 +0100 Subject: [PATCH 05/11] Add node versions to travis config that have http2 --- .travis.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.travis.yml b/.travis.yml index df524d09..40df0040 100644 --- a/.travis.yml +++ b/.travis.yml @@ -10,8 +10,10 @@ node_js: - "5.12" - "6.14" - "7.10" + - "8.7" # Keep 8.7 around because it is the last minor on 8.x that didn't have http2 - "8.11" - "9.11" + - "10.11" sudo: false cache: directories: From eb2e280c064b942a62ea3696b0487a058c1c5b0c Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 18:10:22 +0100 Subject: [PATCH 06/11] Fix port in tests to be assigned automatically --- test/compression.js | 29 +++++++++++++++-------------- 1 file changed, 15 insertions(+), 14 deletions(-) diff --git a/test/compression.js b/test/compression.js index 504f4281..edc1eb52 100644 --- a/test/compression.js +++ b/test/compression.js @@ -311,21 +311,22 @@ describe('compression()', function () { if (http2) { it('should work with http2 server', function (done) { - createHttp2Server({ threshold: 0 }, function (req, res) { + var server = createHttp2Server({ threshold: 0 }, function (req, res) { res.setHeader('Content-Type', 'text/plain') res.end('hello, world') }) - - var client = createHttp2Client() - var request = client.request({ - 'Accept-Encoding': 'gzip' - }) - request.on('response', function (headers) { - assert.equal(headers['content-encoding'], 'gzip') - client.destroy() - done() + server.on('listening', function () { + var client = createHttp2Client(server.address().port) + var request = client.request({ + 'Accept-Encoding': 'gzip' + }) + request.on('response', function (headers) { + assert.equal(headers['content-encoding'], 'gzip') + client.destroy() + done() + }) + request.end() }) - request.end() }) } @@ -702,12 +703,12 @@ function createHttp2Server (opts, fn) { fn(req, res) }) }) - server.listen(8443, '127.0.0.1') + server.listen(0, '127.0.0.1') return server } -function createHttp2Client () { - var client = http2.connect('http://127.0.0.1:8443') +function createHttp2Client (port) { + var client = http2.connect('http://127.0.0.1:' + port) return client } From 8d45e7ec91fb352ab75b813ab5e7a2ee3199102c Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 18:31:45 +0100 Subject: [PATCH 07/11] Change http2 test usage to describe.skip if no http2 available --- test/compression.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/compression.js b/test/compression.js index edc1eb52..e4dcd199 100644 --- a/test/compression.js +++ b/test/compression.js @@ -7,8 +7,10 @@ var http = require('http') var request = require('supertest') var zlib = require('zlib') +var describeHttp2 = describe.skip try { var http2 = require('http2') + describeHttp2 = describe } catch (err) { if (err) { console.log('http2 tests disabled.') @@ -309,7 +311,7 @@ describe('compression()', function () { .expect(200, done) }) - if (http2) { + describeHttp2('http2', function () { it('should work with http2 server', function (done) { var server = createHttp2Server({ threshold: 0 }, function (req, res) { res.setHeader('Content-Type', 'text/plain') @@ -328,7 +330,7 @@ describe('compression()', function () { request.end() }) }) - } + }) describe('threshold', function () { it('should not compress responses below the threshold size', function (done) { From ce237f334ab8aa099c5cdfad12194ffa6df7986e Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 20:17:21 +0100 Subject: [PATCH 08/11] Fix closing the http2 connections to prevent possible exceptions --- test/compression.js | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/compression.js b/test/compression.js index e4dcd199..0bb0d180 100644 --- a/test/compression.js +++ b/test/compression.js @@ -324,8 +324,11 @@ describe('compression()', function () { }) request.on('response', function (headers) { assert.equal(headers['content-encoding'], 'gzip') - client.destroy() - done() + client.close(function () { + server.close(function () { + done() + }) + }) }) request.end() }) From 75794dd2b24b57c33bf2f474d3181bb6c808ddf9 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 20:39:33 +0100 Subject: [PATCH 09/11] Fix closing the request first, then the client, then the server --- test/compression.js | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/test/compression.js b/test/compression.js index 0bb0d180..65586c7c 100644 --- a/test/compression.js +++ b/test/compression.js @@ -324,9 +324,11 @@ describe('compression()', function () { }) request.on('response', function (headers) { assert.equal(headers['content-encoding'], 'gzip') - client.close(function () { - server.close(function () { - done() + request.close(http2.constants.NGHTTP2_NO_ERROR, function () { + client.close(function () { + server.close(function () { + done() + }) }) }) }) From 521863cb435a9ce258fb6f477f031a7ed7e1cec3 Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Mon, 15 Jan 2018 21:03:55 +0100 Subject: [PATCH 10/11] Fix closing for v8.x and v9.x --- test/compression.js | 30 +++++++++++++++++++++++------- 1 file changed, 23 insertions(+), 7 deletions(-) diff --git a/test/compression.js b/test/compression.js index 65586c7c..1c891618 100644 --- a/test/compression.js +++ b/test/compression.js @@ -324,13 +324,7 @@ describe('compression()', function () { }) request.on('response', function (headers) { assert.equal(headers['content-encoding'], 'gzip') - request.close(http2.constants.NGHTTP2_NO_ERROR, function () { - client.close(function () { - server.close(function () { - done() - }) - }) - }) + closeHttp2(request, client, server, done) }) request.end() }) @@ -719,6 +713,28 @@ function createHttp2Client (port) { return client } +function closeHttp2 (request, client, server, callback) { + if (typeof client.shutdown === 'function') { + // this is the node v8.x way of closing the connections + request.destroy(http2.constants.NGHTTP2_NO_ERROR, function () { + client.shutdown({}, function () { + server.close(function () { + callback() + }) + }) + }) + } else { + // this is the node v9.x onwards (hopefully) way of closing the connections + request.close(http2.constants.NGHTTP2_NO_ERROR, function () { + client.close(function () { + server.close(function () { + callback() + }) + }) + }) + } +} + function shouldHaveBodyLength (length) { return function (res) { assert.equal(res.text.length, length, 'should have body length of ' + length) From 19a6ac56c80e67050eeefe68b52059888c9e890f Mon Sep 17 00:00:00 2001 From: Moritz Peters Date: Wed, 26 Sep 2018 16:24:38 +0200 Subject: [PATCH 11/11] fix tests not draining data for http2 requests, resulting in timeouts in node v10.4 onwards --- test/compression.js | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/test/compression.js b/test/compression.js index 1c891618..c087ede9 100644 --- a/test/compression.js +++ b/test/compression.js @@ -324,6 +324,14 @@ describe('compression()', function () { }) request.on('response', function (headers) { assert.equal(headers['content-encoding'], 'gzip') + }) + request.on('error', function (error) { + console.error('An error event occurred on a http2 client request.', error) + }) + request.on('data', function (chunk) { + // no-op without which the request will stay open and cause a test timeout + }) + request.on('end', function () { closeHttp2(request, client, server, done) }) request.end() @@ -694,6 +702,12 @@ function createServer (opts, fn) { function createHttp2Server (opts, fn) { var _compression = compression(opts) var server = http2.createServer(function (req, res) { + req.on('error', function (error) { + console.error('An error event occurred on a http2 request.', error) + }) + res.on('error', function (error) { + console.error('An error event occurred on a http2 response.', error) + }) _compression(req, res, function (err) { if (err) { res.statusCode = err.status || 500 @@ -704,12 +718,21 @@ function createHttp2Server (opts, fn) { fn(req, res) }) }) + server.on('error', function (error) { + console.error('An error event occurred on the http2 server.', error) + }) + server.on('sessionError', function (error) { + console.error('A sessionError event occurred on the http2 server.', error) + }) server.listen(0, '127.0.0.1') return server } function createHttp2Client (port) { var client = http2.connect('http://127.0.0.1:' + port) + client.on('error', function (error) { + console.error('An error event occurred in the http2 client stream.', error) + }) return client } @@ -724,9 +747,14 @@ function closeHttp2 (request, client, server, callback) { }) }) } else { - // this is the node v9.x onwards (hopefully) way of closing the connections + // this is the node v9.x onwards way of closing the connections request.close(http2.constants.NGHTTP2_NO_ERROR, function () { client.close(function () { + // force existing connections to time out after 1ms. + // this is done to force the server to close in some cases where it wouldn't do it otherwise. + server.setTimeout(1, function () { + console.warn('An http2 connection timed out instead of being closed properly.') + }) server.close(function () { callback() })