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

http2,doc: minor fixes #28044

Closed
wants to merge 1 commit into from
Closed

Conversation

Copy link
Member

@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++ http2 labels Jun 3, 2019
doc/api/http2.md Outdated Show resolved Hide resolved
src/node_http2.cc Outdated Show resolved Hide resolved
@mildsunrise
Copy link
Member Author

@mildsunrise mildsunrise commented Jun 5, 2019

(sorry, I closed by accident)

@fhinkel
Copy link
Member

@fhinkel fhinkel commented Nov 1, 2019

ping @jmendeth

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Jan 12, 2020

@HarshithaKP
Copy link
Member

@HarshithaKP HarshithaKP commented Apr 23, 2020

@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
Copy link
Member Author

@mildsunrise mildsunrise commented Apr 27, 2020

Done!

Copy link
Member

@mcollina mcollina left a comment

lgtm

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Apr 28, 2020

@mildsunrise
Copy link
Member Author

@mildsunrise mildsunrise commented Apr 28, 2020

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

@addaleax addaleax commented Apr 28, 2020

Landed in ee9280a :)

addaleax pushed a commit that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@addaleax addaleax closed this Apr 28, 2020
@mildsunrise mildsunrise deleted the feature/http2-fixes branch Apr 28, 2020
targos pushed a commit that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@targos targos mentioned this pull request May 4, 2020
@targos targos added the backport-blocked-v12.x label May 7, 2020
clshortfuse pushed a commit to clshortfuse/node that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
clshortfuse pushed a commit to clshortfuse/node that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
richardlau pushed a commit that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@richardlau richardlau mentioned this pull request Oct 7, 2020
3 tasks
MylesBorins pushed a commit that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Nov 3, 2020
MylesBorins pushed a commit that referenced this issue 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 <matteo.collina@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-blocked-v12.x c++ http2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants