★ wanayoo — archive 1999 https://github.com/nodejs/node/pull/21764Nouvelle recherche | Portail wanayoo
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

Conversation

Projects
None yet
8 participants
@RidgeA
Copy link
Contributor

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

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

This comment has been minimized.

Copy link
Member

Trott commented Jul 11, 2018

@mcollina

This comment has been minimized.

@RidgeA

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

mcollina commented Jul 12, 2018

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

@mcollina
Copy link
Member

mcollina left a comment

LGTM

@BridgeAR
Copy link
Member

BridgeAR left a comment

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);

This comment has been minimized.

@BridgeAR

BridgeAR Jul 12, 2018

Member

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

This comment has been minimized.

@RidgeA

RidgeA Jul 13, 2018

Author Contributor

Done

HTTP_STATUS_RESET_CONTENT,
HTTP_STATUS_NOT_MODIFIED,
];
const STATUS_CODES_COUNT = STATUS_WITHOUT_BODY.length;

This comment has been minimized.

@BridgeAR

BridgeAR Jul 12, 2018

Member

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.

This comment has been minimized.

@RidgeA

RidgeA Jul 13, 2018

Author Contributor

Done.

@Trott

This comment has been minimized.

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

This comment has been minimized.

@addaleax

This comment has been minimized.

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 added a commit that referenced this pull request Jul 15, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

targos added a commit that referenced this pull request Jul 16, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

@targos targos referenced this pull request Jul 17, 2018

Merged

v10.7.0 proposal #21851

kjin added a commit to kjin/node that referenced this pull request Aug 23, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

kjin added a commit to kjin/node that referenced this pull request Sep 17, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

kjin added a commit to kjin/node that referenced this pull request Sep 19, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

kjin added a commit to kjin/node that referenced this pull request Sep 25, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

kjin added a commit to kjin/node that referenced this pull request Oct 16, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

BethGriggs added a commit to BethGriggs/node that referenced this pull request Oct 16, 2018

http2: remove `waitTrailers` listener after closing a stream
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
Backport-PR-URL: nodejs#22850
PR-URL: nodejs#21764
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

BethGriggs added a commit that referenced this pull request Oct 17, 2018

http2: remove `waitTrailers` listener after closing a stream
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 <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>

@BethGriggs BethGriggs referenced this pull request Oct 30, 2018

Merged

v8.13.0 proposal #23974

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.