Skip to content

[v14.x] http2: use and support non-empty DATA frame with END_STREAM flag - #34838

Closed
clshortfuse wants to merge 1 commit into
nodejs:v14.x-stagingfrom
clshortfuse:fixhttp2-v14.x
Closed

[v14.x] http2: use and support non-empty DATA frame with END_STREAM flag#34838
clshortfuse wants to merge 1 commit into
nodejs:v14.x-stagingfrom
clshortfuse:fixhttp2-v14.x

Conversation

@clshortfuse

@clshortfuseclshortfuse commented Aug 19, 2020

Copy link
Copy Markdown
Contributor

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. When writing to a stream, uses a
END_STREAM flag on final DATA frame instead of adding an empty
DATA frame.

BREAKING: http2 client now expects servers to properly support
END_STREAM flag

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

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

@clshortfuse
clshortfuse requested review from a team as code ownersAugust 19, 2020 04:59
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/http2
  • @nodejs/net

@nodejs-github-botnodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. v14.x labels Aug 19, 2020
@clshortfuseclshortfuse changed the title http2: use and support non-empty DATA frame with END_STREAM flaghttp2: (backport v14.x) use and support non-empty DATA frame with END_STREAM flagAug 19, 2020
@clshortfuse

Copy link
Copy Markdown
ContributorAuthor

Only difference between this and #33875 are two debug lines that had to be removed. v14.x can't debug a stream object without an attached session.

@clshortfuse

Copy link
Copy Markdown
ContributorAuthor

Rebased against 14.x-staging (was behind 640 commits). Also commented out 14.x incompatible lines instead of just outright removing them.

@danielleadams

Copy link
Copy Markdown
Contributor

@clshortfuse
clshortfuse requested a review from a teamAugust 20, 2020 14:06
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. When writing to a stream, uses a
END_STREAM flag on final DATA frame instead of adding an empty
DATA frame.
BREAKING: http2 client now expects servers to properly support
END_STREAM flag
Fixes: nodejs#31309Fixes: nodejs#33891
Refs: https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback
@MylesBorins

MylesBorins commented Aug 20, 2020

Copy link
Copy Markdown
Contributor

Went ahead and rebased as the change went out of sync with v14.x-staging

CI: https://ci.nodejs.org/job/node-test-pull-request/32872/

@danielleadams it would be very good to get this included in next weeks release. Thanks for the hard work.

Resume CI for windows failure: https://ci.nodejs.org/job/node-test-pull-request/32873/

@MylesBorinsMylesBorins changed the title http2: (backport v14.x) use and support non-empty DATA frame with END_STREAM flag[v14.x] http2: use and support non-empty DATA frame with END_STREAM flagAug 20, 2020
MylesBorins pushed a commit that referenced this pull request Aug 21, 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. When writing to a stream, uses a
END_STREAM flag on final DATA frame instead of adding an empty
DATA frame.
BREAKING: http2 client now expects servers to properly support
END_STREAM flag
Fixes: #31309Fixes: #33891
Refs: https://nghttp2.org/documentation/types.html#c.nghttp2_on_data_chunk_recv_callback
PR-URL: #33875
Backport-PR-URL: #34838
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

Copy link
Copy Markdown
Contributor

landed in 9e0d18f

Sign up for freeto 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++.lib / srcIssues and PRs related to general changes in the lib or src directory.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@clshortfuse@nodejs-github-bot@danielleadams@MylesBorins