Skip to content

Commit

Permalink
http2: fix closedCode NaN, increase test coverage
Browse files Browse the repository at this point in the history
[kFinish](code) can be triggered from a 'finish' event (for example
when calling response.end) which does not pass code. That tries to
set closedCode to undefined resulting in NaN closedCode instead of
NGHTTP2_NO_ERROR. Check for code !== undefined before setting.
Adds tests for closed and closedCode.

PR-URL: #15154
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
  • Loading branch information
apapirovski authored and jasnell committed Sep 7, 2017
1 parent 7487d61 commit 800c32d
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 2 deletions.
6 changes: 4 additions & 2 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -249,7 +249,8 @@ class Http2ServerRequest extends Readable {
const state = this[kState];
if (state.closed)
return;
state.closedCode = code;
if (code !== undefined)
state.closedCode = code;
state.closed = true;
this.push(null);
this[kStream] = undefined;
Expand Down Expand Up @@ -522,7 +523,8 @@ class Http2ServerResponse extends Stream {
const state = this[kState];
if (state.closed)
return;
state.closedCode = code;
if (code !== undefined)
state.closedCode = code;
state.closed = true;
this.end();
this[kStream] = undefined;
Expand Down
5 changes: 5 additions & 0 deletions test/parallel/test-http2-compat-serverrequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ server.listen(0, common.mustCall(function() {
httpVersionMinor: 0
};

assert.strictEqual(request.closed, false);
assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);

assert.strictEqual(request.statusCode, expected.statusCode);
assert.strictEqual(request.httpVersion, expected.version);
assert.strictEqual(request.httpVersionMajor, expected.httpVersionMajor);
Expand All @@ -31,6 +34,8 @@ server.listen(0, common.mustCall(function() {
assert.strictEqual(request.socket, request.connection);

response.on('finish', common.mustCall(function() {
assert.strictEqual(request.closed, true);
assert.strictEqual(request.code, h2.constants.NGHTTP2_NO_ERROR);
server.close();
}));
response.end();
Expand Down
3 changes: 3 additions & 0 deletions test/parallel/test-http2-compat-serverresponse-headers.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,10 @@ server.listen(0, common.mustCall(function() {
response.sendDate = false;
assert.strictEqual(response.sendDate, false);

assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR);

response.on('finish', common.mustCall(function() {
assert.strictEqual(response.code, h2.constants.NGHTTP2_NO_ERROR);
server.close();
}));
response.end();
Expand Down

0 comments on commit 800c32d

Please sign in to comment.