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

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

Closed
wants to merge 5 commits into from

Commits on Sep 22, 2020

  1. http2: wait for session to finish writing before destroy

    PR-URL: nodejs#30854
    Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Rich Trott <rtrott@gmail.com>
    lundibundi authored and clshortfuse committed Sep 22, 2020
    Configuration menu
    Copy the full SHA
    4e4592b View commit details
    Browse the repository at this point in the history
  2. http2: wait for session socket writable end on close/destroy

    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 <matteo.collina@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Rich Trott <rtrott@gmail.com>
    lundibundi authored and clshortfuse committed Sep 22, 2020
    Configuration menu
    Copy the full SHA
    0453ece View commit details
    Browse the repository at this point in the history
  3. http2,doc: minor fixes

    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 <matteo.collina@gmail.com>
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    mildsunrise authored and clshortfuse committed Sep 22, 2020
    Configuration menu
    Copy the full SHA
    98eea4f View commit details
    Browse the repository at this point in the history

Commits on Oct 7, 2020

  1. Revert "http2: streamline OnStreamRead streamline memory accounting"

    This reverts commit 51ccf1b.
    
    Fixes: nodejs#31089
    
    PR-URL: nodejs#34315
    Reviewed-By: Anna Henningsen <anna@addaleax.net>
    Reviewed-By: David Carlier <devnexen@gmail.com>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    Reviewed-By: Denys Otrishko <shishugi@gmail.com>
    Trott authored and clshortfuse committed Oct 7, 2020
    Configuration menu
    Copy the full SHA
    6802870 View commit details
    Browse the repository at this point in the history
  2. http2: use and support non-empty DATA frame with END_STREAM flag

    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 <anna@addaleax.net>
    Reviewed-By: James M Snell <jasnell@gmail.com>
    clshortfuse committed Oct 7, 2020
    Configuration menu
    Copy the full SHA
    a9ea96c View commit details
    Browse the repository at this point in the history