Skip to content

Commit

Permalink
http2: small fixes to compatibility layer
Browse files Browse the repository at this point in the history 
Expand argument validation through compat API, adjust behaviour
of response.end to not throw if stream already closed to match
http1, adjust behaviour of writeContinue to not throw if stream
already closed and other very small tweaks. Add tests for added
and fixed behaviour. Add tests for edge case behaviours of
setTimeout, createPushResponse, destroy, end and trailers.

PR-URL: nodejs/node#15473
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Minwoo Jung <jmwsoft@gmail.com>
  • Loading branch information
@apapirovski
apapirovski authored and Stephen Belanger committed Sep 21, 2017
1 parent 2b37cad commit 3216116
Show file tree
Hide file tree
Showing 10 changed files with 200 additions and 33 deletions.
38 changes: 19 additions & 19 deletions lib/internal/http2/compat.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ function onStreamError(error) {
// errors in compatibility mode are
// not forwarded to the request
// and response objects. However,
// they are forwarded to 'clientError'
// they are forwarded to 'streamError'
// on the server by Http2Stream
}

Expand Down Expand Up @@ -248,9 +248,9 @@ class Http2ServerRequest extends Readable {
}

setTimeout(msecs, callback) {
const stream = this[kStream];
if (stream === undefined) return;
stream.setTimeout(msecs, callback);
if (this[kState].closed)
return;
this[kStream].setTimeout(msecs, callback);
}

[kFinish](code) {
Expand Down Expand Up @@ -445,7 +445,7 @@ class Http2ServerResponse extends Stream {

if (stream === undefined) {
const err = new errors.Error('ERR_HTTP2_STREAM_CLOSED');
if (cb)
if (typeof cb === 'function')
process.nextTick(cb, err);
else
throw err;
Expand All @@ -461,12 +461,11 @@ class Http2ServerResponse extends Stream {
if (typeof chunk === 'function') {
cb = chunk;
chunk = null;
encoding = 'utf8';
} else if (typeof encoding === 'function') {
cb = encoding;
encoding = 'utf8';
}
if (stream === undefined || stream.finished === true) {
if (this.finished === true) {
return false;
}
if (chunk !== null && chunk !== undefined) {
Expand All @@ -482,21 +481,21 @@ class Http2ServerResponse extends Stream {
}

destroy(err) {
const stream = this[kStream];
if (stream === undefined) {
// nothing to do, already closed
if (this[kState].closed)
return;
}
stream.destroy(err);
this[kStream].destroy(err);
}

setTimeout(msecs, callback) {
const stream = this[kStream];
if (stream === undefined) return;
if (this[kState].closed)
return;
stream.setTimeout(msecs, callback);
}

createPushResponse(headers, callback) {
if (typeof callback !== 'function')
throw new errors.TypeError('ERR_INVALID_CALLBACK');
const stream = this[kStream];
if (stream === undefined) {
process.nextTick(callback, new errors.Error('ERR_HTTP2_STREAM_CLOSED'));
Expand All @@ -513,12 +512,9 @@ class Http2ServerResponse extends Stream {
if (stream !== undefined &&
stream.destroyed === false &&
stream.headersSent === false) {
options = options || Object.create(null);
const state = this[kState];
const headers = this[kHeaders];
headers[HTTP2_HEADER_STATUS] = state.statusCode;
if (stream.finished === true)
options.endStream = true;
headers[HTTP2_HEADER_STATUS] = this[kState].statusCode;
options = options || Object.create(null);
options.getTrailers = (trailers) => {
Object.assign(trailers, this[kTrailers]);
};
Expand All @@ -542,7 +538,11 @@ class Http2ServerResponse extends Stream {
// TODO doesn't support callbacks
writeContinue() {
const stream = this[kStream];
if (stream === undefined) return false;
if (stream === undefined ||
stream.headersSent === true ||
stream.destroyed === true) {
return false;
}
this[kStream].additionalHeaders({
[HTTP2_HEADER_STATUS]: HTTP_STATUS_CONTINUE
});
Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-http2-compat-expect-continue-check.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,11 @@ function handler(req, res) {
'abcd': '1'
});
res.end(testResBody);
// should simply return false if already too late to write
assert.strictEqual(res.writeContinue(), false);
res.on('finish', common.mustCall(
() => process.nextTick(() => assert.strictEqual(res.writeContinue(), false))
));
}

const server = http2.createServer(
Expand Down
15 changes: 13 additions & 2 deletions test/parallel/test-http2-compat-serverrequest-settimeout.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,25 @@
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');

const msecs = common.platformTimeout(1);
const server = http2.createServer();

server.on('request', (req, res) => {
req.setTimeout(common.platformTimeout(1), common.mustCall(() => {
req.setTimeout(msecs, common.mustCall(() => {
res.end();
req.setTimeout(msecs, common.mustNotCall());
}));
res.on('finish', common.mustCall(() => {
req.setTimeout(msecs, common.mustNotCall());
process.nextTick(() => {
assert.doesNotThrow(
() => req.setTimeout(msecs, common.mustNotCall())
);
server.close();
});
}));
});

Expand All @@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => {
':authority': `localhost:${port}`
});
req.on('end', common.mustCall(() => {
server.close();
client.destroy();
}));
req.resume();
Expand Down
5 changes: 4 additions & 1 deletion test/parallel/test-http2-compat-serverrequest.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ server.listen(0, common.mustCall(function() {
response.on('finish', common.mustCall(function() {
assert.strictEqual(request.closed, true);
assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);
server.close();
process.nextTick(() => {
assert.strictEqual(request.socket, undefined);
server.close();
});
}));
response.end();
}));
Expand Down
13 changes: 13 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-createpushresponse.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,19 @@ const server = h2.createServer((request, response) => {
assert.strictEqual(response.stream.id % 2, 1);
response.write(servExpect);

// callback must be specified (and be a function)
common.expectsError(
() => response.createPushResponse({
':path': '/pushed',
':method': 'GET'
}, undefined),
{
code: 'ERR_INVALID_CALLBACK',
type: TypeError,
message: 'Callback must be a function'
}
);

response.createPushResponse({
':path': '/pushed',
':method': 'GET'
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-destroy.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,9 @@ const server = http2.createServer(common.mustCall((req, res) => {
res.on('finish', common.mustCall(() => {
assert.doesNotThrow(() => res.destroy(nextError));
assert.strictEqual(res.closed, true);
process.nextTick(() => {
assert.doesNotThrow(() => res.destroy(nextError));
});
}));

if (req.url !== '/') {
Expand Down
80 changes: 72 additions & 8 deletions test/parallel/test-http2-compat-serverresponse-end.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
const { mustCall, mustNotCall, hasCrypto, skip } = require('../common');
if (!hasCrypto)
skip('missing crypto');
const { doesNotThrow, strictEqual } = require('assert');
const { strictEqual } = require('assert');
const {
createServer,
connect,
Expand All @@ -15,18 +15,82 @@ const {
} = require('http2');

{
// Http2ServerResponse.end callback is called only the first time,
// but may be invoked repeatedly without throwing errors.
// Http2ServerResponse.end accepts chunk, encoding, cb as args
// It may be invoked repeatedly without throwing errors
// but callback will only be called once
const server = createServer(mustCall((request, response) => {
strictEqual(response.closed, false);
response.on('finish', mustCall(() => process.nextTick(
mustCall(() => doesNotThrow(() => response.end('test', mustNotCall())))
)));
response.end('end', 'utf8', mustCall(() => {
strictEqual(response.closed, true);
response.end(mustNotCall());
process.nextTick(() => {
response.end(mustNotCall());
server.close();
});
}));
response.end(mustNotCall());
}));
server.listen(0, mustCall(() => {
let data = '';
const { port } = server.address();
const url = `http://localhost:${port}`;
const client = connect(url, mustCall(() => {
const headers = {
':path': '/',
':method': 'GET',
':scheme': 'http',
':authority': `localhost:${port}`
};
const request = client.request(headers);
request.setEncoding('utf8');
request.on('data', (chunk) => (data += chunk));
request.on('end', mustCall(() => {
strictEqual(data, 'end');
client.destroy();
}));
request.end();
request.resume();
}));
}));
}

{
// Http2ServerResponse.end can omit encoding arg, sets it to utf-8
const server = createServer(mustCall((request, response) => {
response.end('test\uD83D\uDE00', mustCall(() => {
server.close();
}));
}));
server.listen(0, mustCall(() => {
let data = '';
const { port } = server.address();
const url = `http://localhost:${port}`;
const client = connect(url, mustCall(() => {
const headers = {
':path': '/',
':method': 'GET',
':scheme': 'http',
':authority': `localhost:${port}`
};
const request = client.request(headers);
request.setEncoding('utf8');
request.on('data', (chunk) => (data += chunk));
request.on('end', mustCall(() => {
strictEqual(data, 'test\uD83D\uDE00');
client.destroy();
}));
request.end();
request.resume();
}));
}));
}

{
// Http2ServerResponse.end can omit chunk & encoding args
const server = createServer(mustCall((request, response) => {
response.end(mustCall(() => {
server.close();
}));
response.end(mustNotCall());
strictEqual(response.closed, true);
}));
server.listen(0, mustCall(() => {
const { port } = server.address();
Expand Down
15 changes: 14 additions & 1 deletion test/parallel/test-http2-compat-serverresponse-headers.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,14 @@ server.listen(0, common.mustCall(function() {
message: 'The "name" argument must be of type string'
}
);
common.expectsError(
() => response.setHeader(''),
{
code: 'ERR_INVALID_HTTP_TOKEN',
type: TypeError,
message: 'Header name must be a valid HTTP token [""]'
}
);

response.setHeader(real, expectedValue);
const expectedHeaderNames = [real];
Expand All @@ -122,7 +130,12 @@ server.listen(0, common.mustCall(function() {
response.on('finish', common.mustCall(function() {
assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR);
assert.strictEqual(response.headersSent, true);
server.close();
process.nextTick(() => {
// can access headersSent after stream is undefined
assert.strictEqual(response.stream, undefined);
assert.strictEqual(response.headersSent, true);
server.close();
});
}));
response.end();
}));
Expand Down
15 changes: 13 additions & 2 deletions test/parallel/test-http2-compat-serverresponse-settimeout.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,25 @@
const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const assert = require('assert');
const http2 = require('http2');

const msecs = common.platformTimeout(1);
const server = http2.createServer();

server.on('request', (req, res) => {
res.setTimeout(common.platformTimeout(1), common.mustCall(() => {
res.setTimeout(msecs, common.mustCall(() => {
res.end();
res.setTimeout(msecs, common.mustNotCall());
}));
res.on('finish', common.mustCall(() => {
res.setTimeout(msecs, common.mustNotCall());
process.nextTick(() => {
assert.doesNotThrow(
() => res.setTimeout(msecs, common.mustNotCall())
);
server.close();
});
}));
});

Expand All @@ -24,7 +36,6 @@ server.listen(0, common.mustCall(() => {
':authority': `localhost:${port}`
});
req.on('end', common.mustCall(() => {
server.close();
client.destroy();
}));
req.resume();
Expand Down
Loading

0 comments on commit 3216116

Please sign in to comment.