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

http2: remove waitTrailers listener after closing a stream #21764

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
http2: remove waitTrailers listener after closing a stream
When `writeHear` of `Http2ServerResponse` instance are called with 204,
205 and 304 status codes an underlying stream closes.
If call `end` method after sending any of these status codes it will
cause an error `TypeError: Cannot read property 'Symbol(trailers)' of
undefined` because a reference to `Http2ServerResponse` instance
associated with Http2Stream already was deleted.
The closing of stream causes emitting `watiTrailers` event and, when
this event handles inside `onStreamTrailerReady`  handler, there is
no reference to Http2ServerResponse instance.

Fises: #21740
  • Loading branch information
RidgeA committed Jul 11, 2018
commit 5222ec209e20ec9af55471156e7787367f81b1bc
2 changes: 2 additions & 0 deletions lib/internal/http2/compat.js
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,8 @@ function onStreamCloseResponse() {
state.closed = true;

this[kProxySocket] = null;

this.off('wantTrailers', onStreamTrailersReady);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you be so kind and use removeListener instead? That seems much clearer from the semantics and is therefore easier to grasp :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

this[kResponse] = undefined;

res.emit('finish');
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
'use strict';

const common = require('../common');
if (!common.hasCrypto)
common.skip('missing crypto');
const h2 = require('http2');

// This test case ensures that calling of res.end after sending
// 204, 205 and 304 HTTP statuses will not cause an error
// See issue: https://github.com/nodejs/node/issues/21740

const {
HTTP_STATUS_NO_CONTENT,
HTTP_STATUS_RESET_CONTENT,
HTTP_STATUS_NOT_MODIFIED
} = h2.constants;

const STATUS_WITHOUT_BODY = [
HTTP_STATUS_NO_CONTENT,
HTTP_STATUS_RESET_CONTENT,
HTTP_STATUS_NOT_MODIFIED,
];
const STATUS_CODES_COUNT = STATUS_WITHOUT_BODY.length;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: would you mind changing the STATUS_WITHOUT_BODY name to statusWithoutBody? Since it is an array that is actually manipulated it is not really a constant. And constants in Node.js are normally written as: e.g. kStatusCodesCount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.


const server = h2.createServer(common.mustCall(function(req, res) {
res.writeHead(STATUS_WITHOUT_BODY.pop());
res.end();
}, STATUS_CODES_COUNT));

server.listen(0, common.mustCall(function() {
const url = `http://localhost:${server.address().port}`;
const client = h2.connect(url, common.mustCall(() => {
let responseCount = 0;
const closeAfterResponse = () => {
if (STATUS_CODES_COUNT === ++responseCount) {
client.destroy();
server.close();
}
};

for (let i = 0; i < STATUS_CODES_COUNT; i++) {
const request = client.request();
request.on('response', common.mustCall(closeAfterResponse));
}

}));
}));