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

[v10.x] http2: support non-empty DATA frame with END_STREAM flag #34857

Closed
wants to merge 2 commits into from

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Aug 20, 2020

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

Note: This change only adds supports for HTTP2 server environments to support the END_STREAM flag and no longer hang. HTTP2 clients will not pack the END_STREAM flag as done in from the original cherry-picked commit. This is due to the fact that, both, this would constitute a break change when v10.x is in maintenance LTS mode, and also, it's very complex to patch that functionality into the code.

mildsunrise and others added 2 commits August 20, 2020 13:53
Some small fixes on HTTP/2 and its documentation:

 - Add a note that, on server streams, it's not necessary
   to start data flow.

 - Set EOF flag if we have marked all data for sending:
   there's no need to wait until the queue is
   actually empty (and send a separate, empty DATA).

   (Note that, even with this change, a separate DATA
   frame will always be sent, because the streams
   layer waits until data has been flushed before
   dispatching EOF)

PR-URL: nodejs#28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Adds support for reading from a stream where the final frame is a
non-empty DATA frame with the END_STREAM flag set, instead of hanging
waiting for another frame.

Fixes: nodejs#31309
Refs: https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback

PR-URL: nodejs#33875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/http
  • @nodejs/http2
  • @nodejs/net

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem. v10.x labels Aug 20, 2020
@clshortfuse
Copy link
Contributor Author

I manually confirmed it works with curl --insecure https://localhost:8443/post -d 'test' on Mac OS X against the reproducible code found at #31309 (comment)

@MylesBorins MylesBorins changed the title http2: [v10.x] support non-empty DATA frame with END_STREAM flag [v10.x] http2: support non-empty DATA frame with END_STREAM flag Aug 20, 2020
@richardlau richardlau added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2020
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

richardlau pushed a commit that referenced this pull request Oct 7, 2020
Some small fixes on HTTP/2 and its documentation:

 - Add a note that, on server streams, it's not necessary
   to start data flow.

 - Set EOF flag if we have marked all data for sending:
   there's no need to wait until the queue is
   actually empty (and send a separate, empty DATA).

   (Note that, even with this change, a separate DATA
   frame will always be sent, because the streams
   layer waits until data has been flushed before
   dispatching EOF)

PR-URL: #28044
Backport-PR-URL: #34857
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
richardlau pushed a commit that referenced this pull request Oct 7, 2020
Adds support for reading from a stream where the final frame is a
non-empty DATA frame with the END_STREAM flag set, instead of hanging
waiting for another frame.

Fixes: #31309
Refs: https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback

PR-URL: #33875
Backport-PR-URL: #34857
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@richardlau
Copy link
Member

Landed in 54c2bc2...e9e86e1.

@richardlau richardlau closed this Oct 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants