Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix usage of undocumented _implicitHeader #128

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions .travis.yml
Expand Up @@ -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:
Expand Down
4 changes: 2 additions & 2 deletions index.js
Expand Up @@ -81,7 +81,7 @@ function compression (options) {
}

if (!this._header) {
this._implicitHeader()
this.writeHead(this.statusCode)
}

return stream
Expand All @@ -100,7 +100,7 @@ function compression (options) {
length = chunkLength(chunk, encoding)
}

this._implicitHeader()
this.writeHead(this.statusCode)
}

if (!stream) {
Expand Down
102 changes: 102 additions & 0 deletions test/compression.js
Expand Up @@ -7,6 +7,16 @@ 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.')
}
}

var compression = require('..')

describe('compression()', function () {
Expand Down Expand Up @@ -301,6 +311,34 @@ describe('compression()', function () {
.expect(200, done)
})

describeHttp2('http2', function () {
it('should work with http2 server', function (done) {
dougwilson marked this conversation as resolved.
Show resolved Hide resolved
var server = createHttp2Server({ threshold: 0 }, function (req, res) {
res.setHeader('Content-Type', 'text/plain')
res.end('hello, world')
})
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')
})
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()
})
})
})

describe('threshold', function () {
it('should not compress responses below the threshold size', function (done) {
var server = createServer({ threshold: '1kb' }, function (req, res) {
Expand Down Expand Up @@ -661,6 +699,70 @@ 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
res.end(err.message)
return
}

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
}

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 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()
})
})
})
}
}

function shouldHaveBodyLength (length) {
return function (res) {
assert.equal(res.text.length, length, 'should have body length of ' + length)
Expand Down