Skip to content

Conversation

mildsunrise
Copy link
Contributor

@mildsunrise mildsunrise commented Jun 3, 2019

Some small fixes on HTTP/2 and its documentation:

  • Fix server side example: start data flow when receiving a stream
    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, an empty DATA frame will always be sent, because the streams layer waits until data has been flushed before queueing EOF)

  • Fix debug message

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 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. labels Jun 3, 2019
@mildsunrise
Copy link
Contributor Author

mildsunrise commented Jun 5, 2019

(sorry, I closed by accident)

@fhinkel
Copy link
Member

fhinkel commented Nov 1, 2019

ping @jmendeth

@nodejs-github-bot
Copy link
Collaborator

@HarshithaKP
Copy link
Member

@mildsunrise, this needs a rebase.

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)
@mildsunrise mildsunrise force-pushed the feature/http2-fixes branch from 6905784 to 4c36cd1 Compare April 27, 2020 01:19
@mildsunrise
Copy link
Contributor Author

Done!

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@nodejs-github-bot
Copy link
Collaborator

@mildsunrise
Copy link
Contributor Author

for reference, here's what a response currently looks like:

screenshot showing two DATA frames

after the fix:

screenshot showing one DATA frame

using the following code (this PR isn't enough to make the optimization visible, we still need a call to _final() for now):

http2.createServer().on('stream', (stream, headers) => {
  stream.end('hello');
  stream._final(() => {});
}).listen(8000);

@addaleax
Copy link
Member

Landed in ee9280a :)

addaleax pushed a commit that referenced this pull request Apr 28, 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
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@addaleax addaleax closed this Apr 28, 2020
@mildsunrise mildsunrise deleted the feature/http2-fixes branch April 28, 2020 19:06
targos pushed a commit that referenced this pull request May 4, 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
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@targos targos mentioned this pull request May 4, 2020
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request Aug 20, 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: nodejs#28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
clshortfuse pushed a commit to clshortfuse/node that referenced this pull request Sep 22, 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: nodejs#28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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 richardlau mentioned this pull request Oct 7, 2020
3 tasks
MylesBorins pushed a commit that referenced this pull request Oct 13, 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)

Backport-PR-URL: #34845
PR-URL: #28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this pull request Nov 16, 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)

Backport-PR-URL: #34845
PR-URL: #28044
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
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.

8 participants