http2: emit response event instead of trailers event

[MoonBall]

Fix#41251.

Should emit response event if cat == NGHTTP2_HCAT_HEADERS and status >= 200.

[nodejs-github-bot]

Review requested:

• @nodejs/http2
• @nodejs/net

[apapirovski]

Very nice find. And actually makes sense after reading the nghttp2 docs https://nghttp2.org/documentation/enums.html#c.nghttp2_headers_category

[szmarczak]

The naming is very confusing. HTTP/2 states that those are trailers (since HEADERS are sent with an END_STREAM flag), nghttp2 defines NGHTTP2_HCAT_RESPONSE (informational) + NGHTTP2_HCAT_HEADERS (final) and Node.js would emit response (however it also has a trailers event).

[mcollina]

lgtm

[mcollina]

The naming is very confusing. HTTP/2 states that those are trailers (since HEADERS are sent with an END_STREAM flag), nghttp2 defines NGHTTP2_HCAT_RESPONSE (informational) + NGHTTP2_HCAT_HEADERS (final) and Node.js would emit response (however it also has a trailers event).

Is this an objection? How would you like this PR to be changed?

[szmarczak]

In the Examples section of RFC7540:

 HTTP/1.1 100 Continue HEADERS
Extension-Field: bar ==> - END_STREAM
+ END_HEADERS
:status = 100
extension-field = bar
HTTP/1.1 200 OK HEADERS
Content-Type: image/jpeg ==> - END_STREAM
Transfer-Encoding: chunked + END_HEADERS
Trailer: Foo :status = 200
content-length = 123
123 content-type = image/jpeg
{binary data} trailer = Foo
0
Foo: bar DATA
- END_STREAM
{binary data}
HEADERS
+ END_STREAM
+ END_HEADERS
foo = bar

in this instance there are three HEADERS frames. I think it would make sense to have an information event, like in HTTP/1 API. What do you think?

[aduh95]

This needs a rebase.

[aduh95]

This still needs a rebase :)

[aduh95]

Superseded by #41739