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.

Sign up for GitHub

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

Jump to bottom

http2: small fixes to compatibility layer #15473

Closed
wants to merge 2 commits into from
Closed

http2: small fixes to compatibility layer #15473

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ddd012
http2: small fixes to compatibility layer
apapirovski Sep 19, 2017
506f9d1
adjust settimeout behaviour after stream closed
apapirovski Sep 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 19 additions & 19 deletions lib/internal/http2/compat.js
View file
Open in desktop
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;
Copy link
Member Author

@apapirovski apapirovski Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I could not find any reason that this would be needed. All our tests pass without it and if it is truly needed then there should be a test to cover it. Would appreciate any input!

(I'm referencing lines 520 & 521 above, 519 was just adjusted.)

Copy link
Member

Choose a reason for hiding this comment

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

The stream.finished check here is an optimization to avoid sending an unnecessary empty DATA frame.

Copy link
Member Author

Choose a reason for hiding this comment

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

@jasnell Right but what's the situation where stream.finished could be true in this particular code? We don't have any test cases that generate a situation like this (as you can see in the coverage since this code path is never hit).

Copy link
Member Author

@apapirovski apapirovski Sep 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Hmm... is stream.finished even a thing? As far as I can tell, it's just always undefined. There's this.finished or stream._writableState.finished. Can they even respond in either of those cases?

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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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