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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 5 commits into from
Closed

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

wants to merge 5 commits into from

Conversation

clshortfuse
Copy link
Contributor

@clshortfuse clshortfuse commented Aug 19, 2020 •
edited
Loading

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

@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++. lib / src Issues and PRs related to general changes in the lib or src directory. v12.x labels Aug 19, 2020
@MylesBorins MylesBorins changed the title http2: (backport v12.x) use and support non-empty DATA frame with END_STREAM flag [v12.x] http2: use and support non-empty DATA frame with END_STREAM flag Aug 20, 2020
@clshortfuse clshortfuse requested review from a team September 22, 2020 08:48
@addaleax addaleax force-pushed the v12.x-staging branch 4 times, most recently from 55fe022 to 65b7bf4 Compare September 22, 2020 17:57
@addaleax
Copy link
Member

@clshortfuse Sorry for the long waiting time, but could you rebase this? If not, I can do that for you as well

lundibundi and others added 3 commits September 22, 2020 15:10
@lundibundi @clshortfuse
http2: wait for session to finish writing before destroy
4e4592b 
PR-URL: nodejs#30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@lundibundi @clshortfuse
http2: wait for session socket writable end on close/destroy
0453ece 
This slightly alters the behaviour of session close by first using
.end() on a session socket to finish writing the data and only then
calls .destroy() to make sure the Readable side is closed. This allows
the socket to finish transmitting data, receive proper FIN packet and
avoid ECONNRESET errors upon graceful close.

onStreamClose now directly calls stream.destroy() instead of
kMaybeDestroy because the latter will first check that the stream has
writableFinished set. And that may not be true as we have just
(synchronously) called .end() on the stream if it was not closed and
that doesn't give it enough time to finish. Furthermore there is no
point in waiting for 'finish' as the other party have already closed the
stream and we won't be able to write anyway.

This also changes a few tests to correctly handle graceful session
close. This includes:
* not reading request data (on client side)
* not reading push stream data (on client side)
* relying on socket.destroy() (on client) to finish server session
  due to the destroy of the socket without closing the server session.
  As the goaway itself is *not* a session close.

Added few 'close' event mustCall checks.

PR-URL: nodejs#30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@mildsunrise @clshortfuse
http2,doc: minor fixes
98eea4f 
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]>
@clshortfuse
Copy link
Contributor Author

@addaleax Rebased

@addaleax addaleax added the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Sep 23, 2020
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@addaleax
Copy link
Member

I’ve kicked off another CI but it looks like the failures here might have been related…

@clshortfuse
Copy link
Contributor Author

I'll review the failures with the research from #31089

@niftylettuce
Copy link

Any luck here @clshortfuse?

@MylesBorins
Copy link
Contributor

It looks like the fix for 10.x landed but this hasn't yet, what is blocking it here?

Trott and others added 2 commits October 7, 2020 14:18
@Trott @clshortfuse
Revert "http2: streamline OnStreamRead streamline memory accounting"
6802870 
This reverts commit 51ccf1b.

Fixes: nodejs#31089

PR-URL: nodejs#34315
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
@clshortfuse
http2: use and support non-empty DATA frame with END_STREAM flag
a9ea96c 
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#31309
Fixes: nodejs#33891
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]>
@clshortfuse
Copy link
Contributor Author

@addaleax The added cherry-pick fixed the issue. All tests are passing now.

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

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

@nemphys
Copy link

nemphys commented Oct 12, 2020

Any news on this one?

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
@lundibundi @MylesBorins
http2: wait for session to finish writing before destroy
1cdf5d8 
Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
@lundibundi @MylesBorins
http2: wait for session socket writable end on close/destroy
c950e76 
This slightly alters the behaviour of session close by first using
.end() on a session socket to finish writing the data and only then
calls .destroy() to make sure the Readable side is closed. This allows
the socket to finish transmitting data, receive proper FIN packet and
avoid ECONNRESET errors upon graceful close.

onStreamClose now directly calls stream.destroy() instead of
kMaybeDestroy because the latter will first check that the stream has
writableFinished set. And that may not be true as we have just
(synchronously) called .end() on the stream if it was not closed and
that doesn't give it enough time to finish. Furthermore there is no
point in waiting for 'finish' as the other party have already closed the
stream and we won't be able to write anyway.

This also changes a few tests to correctly handle graceful session
close. This includes:
* not reading request data (on client side)
* not reading push stream data (on client side)
* relying on socket.destroy() (on client) to finish server session
  due to the destroy of the socket without closing the server session.
  As the goaway itself is *not* a session close.

Added few 'close' event mustCall checks.

Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
@mildsunrise @MylesBorins
http2,doc: minor fixes
6b5ec36 
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)

Backport-PR-URL: #34845
PR-URL: #28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
@Trott @MylesBorins
Revert "http2: streamline OnStreamRead streamline memory accounting"
b5bc280 
This reverts commit 51ccf1b.

Fixes: #31089

Backport-PR-URL: #34845
PR-URL: #34315
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins pushed a commit that referenced this pull request Oct 13, 2020
@clshortfuse @MylesBorins
http2: use and support non-empty DATA frame with END_STREAM flag
4d1df84 
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

Backport-PR-URL: #34845
PR-URL: #33875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

landed in 6a2f050...4d1df84

MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@lundibundi @MylesBorins
http2: wait for session to finish writing before destroy
2471197 
Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@lundibundi @MylesBorins
http2: wait for session socket writable end on close/destroy
e85ca7a 
This slightly alters the behaviour of session close by first using
.end() on a session socket to finish writing the data and only then
calls .destroy() to make sure the Readable side is closed. This allows
the socket to finish transmitting data, receive proper FIN packet and
avoid ECONNRESET errors upon graceful close.

onStreamClose now directly calls stream.destroy() instead of
kMaybeDestroy because the latter will first check that the stream has
writableFinished set. And that may not be true as we have just
(synchronously) called .end() on the stream if it was not closed and
that doesn't give it enough time to finish. Furthermore there is no
point in waiting for 'finish' as the other party have already closed the
stream and we won't be able to write anyway.

This also changes a few tests to correctly handle graceful session
close. This includes:
* not reading request data (on client side)
* not reading push stream data (on client side)
* relying on socket.destroy() (on client) to finish server session
  due to the destroy of the socket without closing the server session.
  As the goaway itself is *not* a session close.

Added few 'close' event mustCall checks.

Backport-PR-URL: #34845
PR-URL: #30854
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@mildsunrise @MylesBorins
http2,doc: minor fixes
82af8ac 
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)

Backport-PR-URL: #34845
PR-URL: #28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@Trott @MylesBorins
Revert "http2: streamline OnStreamRead streamline memory accounting"
bfce0eb 
This reverts commit 51ccf1b.

Fixes: #31089

Backport-PR-URL: #34845
PR-URL: #34315
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: David Carlier <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
MylesBorins pushed a commit that referenced this pull request Nov 16, 2020
@clshortfuse @MylesBorins
http2: use and support non-empty DATA frame with END_STREAM flag
b732c92 
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

Backport-PR-URL: #34845
PR-URL: #33875
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@clshortfuse @nodejs-github-bot @addaleax @niftylettuce @MylesBorins @nemphys @lundibundi @mildsunrise @Trott