Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign uphttp2: remove `waitTrailers` listener after closing a stream #21764
Conversation
nodejs-github-bot
added
dont-land-on-v4.x
dont-land-on-v6.x
http2
labels
Jul 11, 2018
RidgeA
referenced this pull request
Jul 11, 2018
Closed
HTTP2: Calling `res.end` after `res.writeHead(204)` cause an error #21740
This comment has been minimized.
This comment has been minimized.
jasnell
approved these changes
Jul 11, 2018
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Did I break something in FreeBSD? |
This comment has been minimized.
This comment has been minimized.
|
Very likely it's a flaky test. I've resumed the build. |
BridgeAR
approved these changes
Jul 12, 2018
|
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.
This comment has been minimized.
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.
This comment has been minimized.
| HTTP_STATUS_RESET_CONTENT, | ||
| HTTP_STATUS_NOT_MODIFIED, | ||
| ]; | ||
| const STATUS_CODES_COUNT = STATUS_WITHOUT_BODY.length; |
This comment has been minimized.
This comment has been minimized.
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.
This comment has been minimized.
BridgeAR
added
the
author ready
label
Jul 12, 2018
This comment has been minimized.
This comment has been minimized.
|
CI that @mcollina started via Resume Build is all green: https://ci.nodejs.org/job/node-test-pull-request/15834/ |
RidgeA
added some commits
Jul 13, 2018
This comment has been minimized.
This comment has been minimized.
apapirovski
approved these changes
Jul 15, 2018
This comment has been minimized.
This comment has been minimized.
|
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! |
RidgeA commentedJul 11, 2018
When
writeHearofHttp2ServerResponseinstance are called with 204,205 and 304 status codes an underlying stream closes.
If call
endmethod after sending any of these status codes it willcause an error
TypeError: Cannot read property 'Symbol(trailers)' of undefinedbecause a reference toHttp2ServerResponseinstanceassociated with Http2Stream already was deleted.
The closing of stream causes emitting
watiTrailersevent and, whenthis event handles inside
onStreamTrailerReadyhandler, there isno reference to Http2ServerResponse instance.
Fises: #21740
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAfter 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