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

http2: remove waitTrailers listener after closing a stream #21764

wants to merge 3 commits into from

Conversation

RidgeA
Copy link
Contributor

@RidgeA RidgeA commented Jul 11, 2018

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

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

After applying this changes trailing headers won't be sent if 204/205 and 204 status has been sent.
However, I don't think It is a problem, as according to standard for this HTTP status codes server shouldn't generate payload and have to close a connection after sending the blank line terminating the header section.

204 - https://tools.ietf.org/html/rfc7231#section-6.3.5
205 - https://tools.ietf.org/html/rfc7231#section-6.3.6
304 - https://tools.ietf.org/html/rfc7232#section-4.1

Additional info:
https://tools.ietf.org/html/rfc7230#section-3.3.3 (point 1)
https://tools.ietf.org/html/rfc7540#section-8.1.2.4

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
@Trott
Copy link
Member

Trott commented Jul 11, 2018

@nodejs/http2

@mcollina
Copy link
Member

@RidgeA
Copy link
Contributor Author

RidgeA commented Jul 12, 2018

Did I break something in FreeBSD?
If It it is my fault - let me know, please.

@mcollina
Copy link
Member

Very likely it's a flaky test. I've resumed the build.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

LGTM. My nits do not have to be addressed but it would still be nice :)

@@ -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

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.

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 12, 2018
@Trott
Copy link
Member

Trott commented Jul 12, 2018

CI that @mcollina started via Resume Build is all green: https://ci.nodejs.org/job/node-test-pull-request/15834/

@mcollina
Copy link
Member

@addaleax
Copy link
Member

addaleax commented Jul 15, 2018

Landed in 8babbc5 (only fixed up minor typos in the commit message and the test name while landing)

Thanks for the PR! 🎉

Edit: Oh, by the way – this was commit no 23000 in this repo! ✨

@addaleax addaleax closed this Jul 15, 2018
addaleax pushed a commit that referenced this pull request Jul 15, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: #21740
PR-URL: #21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
targos pushed a commit that referenced this pull request Jul 16, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: #21740
PR-URL: #21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@targos targos mentioned this pull request Jul 17, 2018
kjin pushed a commit to kjin/node that referenced this pull request Aug 23, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: nodejs#21740
PR-URL: nodejs#21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Sep 25, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: nodejs#21740
PR-URL: nodejs#21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
kjin pushed a commit to kjin/node that referenced this pull request Oct 16, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: nodejs#21740
PR-URL: nodejs#21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
BethGriggs pushed a commit that referenced this pull request Oct 17, 2018
When `writeHeader` 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 `waitTrailers` event and, when
this event handles inside `onStreamTrailerReady` handler, there is
no reference to Http2ServerResponse instance.

Fixes: #21740
Backport-PR-URL: #22850
PR-URL: #21764
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 30, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants