Skip to content
Prev Previous commit
fix tests not draining data for http2 requests, resulting in timeouts…
… in node v10.4 onwards
  • Loading branch information
maritz committed Sep 27, 2018
commit 19a6ac56c80e67050eeefe68b52059888c9e890f
30 changes: 29 additions & 1 deletion test/compression.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
request.on('data', function (chunk) {
// no-op without which the request will stay open and cause a test timeout
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't this assert that the data was actually correct?

})
request.on('end', function () {
closeHttp2(request, client, server, done)
})
request.end()
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
res.on('error', function (error) {
console.error('An error event occurred on a http2 response.', error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
_compression(req, res, function (err) {
if (err) {
res.statusCode = err.status || 500
Expand All @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
server.on('sessionError', function (error) {
console.error('A sessionError event occurred on the http2 server.', error)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No one is going to comb through the CI logs after every run or upgrade... do these need to be here vs just letting an error crash the process? If they are preventing an expected error, it shouldn't be logging to the screen, as there should be no extra output when everything was actually OK.

})
return client
}

Expand All @@ -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.')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the comment says "force existing connections to time out after 1ms.", but inside the timeout there is nothing but a console warn. Should there be something being don in here? Otherwise, what purpose does the timeout serve? If to fail the test, should an error be passed to callback instead to make the test fail?

})
server.close(function () {
callback()
})
Expand Down