http2: custom priority for push streams

[sebdeckers]

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:

• Priority of pushed streams nghttp2/nghttp2#429
• Add API to change stream priority without sending PRIORITY frame nghttp2/nghttp2#430

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

[jasnell]

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

[sebdeckers]

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]

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]

From the nghttp2 docs:

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

[apapirovski]

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]

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

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

[apapirovski]

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

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

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]

Closing in favour of #16451