FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

http2: custom priority for push streams by sebdeckers · Pull Request #16444 · nodejs/node · GitHub

/ node Public

http2: custom priority for push streams - #16444

Closed
sebdeckers wants to merge 2 commits into
nodejs:masterfrom
sebdeckers:push-stream-priority
ConversationCommits (2)ChecksFiles changed
Closed

http2: custom priority for push streams#16444
sebdeckers wants to merge 2 commits into
nodejs:masterfrom
sebdeckers:push-stream-priority

Conversation

sebdeckers commented Oct 24, 2017

Copy link
Copy Markdown
Contributor

Sets the priority silently on push streams. Previously the weight options, whilst documented, was simply ignored.

Note that the client is not informed of the non-default priority. The nghttp2 library ignores push stream flags (e.g. 0x20 for priority). It also currently offers no way to set priority value during push stream creation. This appears to be by design, as discussed here:

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
Affected core subsystem(s)

http2

http2: custom priority for push streams
50d3088 
Sets the priority silently on push streams. Previously the weight options, whilst documented, was simply ignored.

Note that the client is not informed of the non-default priority. The nghttp2 library ignores push stream flags (e.g. 0x20 for priority). It also currently offers no way to set priority value during push stream creation. This appears to be by design, as discussed here:
- nghttp2/nghttp2#429
- nghttp2/nghttp2#430
sebdeckers added the http2 Issues or PRs related to the http2 subsystem. label Oct 24, 2017
nodejs-github-bot added dont-land-on-v4.x http2 Issues or PRs related to the http2 subsystem. labels Oct 24, 2017
sebdeckers requested a review from jasnell October 24, 2017 15:59
jasnell reviewed Oct 24, 2017
View reviewed changes
Comment thread test/parallel/test-http2-server-push-priority.js
const req = client.request(headers);

client.on('stream', common.mustCall((stream, headers, flags) => {
// Since the push priority is set silently, the client is not informed.

jasnell Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

there is a priority event that should be emitted, I believe.

sebdeckers Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The issue here is that on the wire I wasn't seeing a priority flag on the headers frame (push request), nor a separate priority frame. This is different from client-initiated streams.

jasnell Oct 24, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

oh right right... now I recall... this is actually a bit of a grey area in the current http2 specification: https://tools.ietf.org/html/rfc7540#section-5.3 ... it's actually not clear at all if the server is even allowed to set the priority on a push stream.

sebdeckers Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

From the nghttp2 docs:

nghttp2_submit_push_promise [...]
The flags is currently ignored.

apapirovski approved these changes Oct 24, 2017
View reviewed changes

apapirovski left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM but I would maybe remove the commented out assertions since there are comments that explain what's going on. The ones on the client in particular since currently priority on push streams is server-only and not announced.

sebdeckers commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@apapirovski Thanks! Happy to remove the commented asserts. They were meant to demonstrate the problem in nghttp2 (AFAIK). Ideally a solution could be found to set the priority field (i.e. our "weight" option) on the outgoing HEADERS frame, if that's possible.

jasnell commented Oct 24, 2017

Copy link
Copy Markdown
Member

I'm thinking that for now we actually should remove the weight options from pushStream.

apapirovski commented Oct 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

@jasnell it seems somewhat useful to have it for the server itself, even if it's not being communicated to the client. Unless I'm misunderstanding what nghttp2 is doing with it.

I would almost lean towards having assertions in that test on the client that confirm that there's no priority flag and the weight isn't being changed.

sebdeckers commented Oct 24, 2017

Copy link
Copy Markdown
Contributor Author

@jasnell Makes sense; only need to remove the option from the docs in that case. Should I do that here or do I close this PR and open another with the doc fix?

@apapirovski It simply wasn't working anyway before this patch. And a user can continue to use stream.priority({weight}) after instantiating the stream. The spec uses language suggesting only the "client" can set a priority in the HEADERS frame. However nothing stops a server from sending a PRIORITY frame. This does seem wasteful to me, as it emits additional frames, and there is little a client can do with that information anyway. Setting the priority silently should be enough to let nghttp2 allocate bandwidth across concurrent streams.

apapirovski commented Oct 24, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor

It simply wasn't working anyway before this patch. And a user can continue to use stream.priority({weight}) after instantiating the stream.

Yeah, I'm just talking in relation to this particular case and test. I feel like it makes sense to document and assert within this test that the priority information isn't going across the wire when using the weight option. (Like if nghttp2 ever changes its behaviour or we do something else weird like remove silent: true.)

That said, if @jasnell still prefers to remove then that's fine but to me this silent priority seems preferable. It takes away the need for the end-user to understand that push streams shouldn't send priority frames and likely gets them what they wanted.

Hope that made sense :)

sebdeckers mentioned this pull request Oct 24, 2017
Closed
2 tasks

sebdeckers commented Oct 25, 2017

Copy link
Copy Markdown
Contributor Author

Closing in favour of #16451

sebdeckers closed this Oct 25, 2017
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
+1 more reviewer
apapirovski apapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

http2 Issues or PRs related to the http2 subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL