HTTP/2: implement RFC 9218 Extensible Prioritization Scheme for HTTP. #552
Conversation
|
@arturobernalg Before we can do anything of this kind we must ensure conformance with RFC 9113 first. |
ok2c
force-pushed
the
master
branch
4 times, most recently
from
8fb1d8e to
bf7d524
Compare
|
@arturobernalg This is a big change. I have no bandwidth to carefully read through RFC 9218 at the moment. What I can do to review your changes for impact on the existing code and APIs. The rest becomes your area of responsibility, if you are willing to maintain this feature. Please also consider how stable it is and whether one release cycle should be enough to stabilize it or it should be marked experimental and go though several release cycles. |
arturobernalg
force-pushed
the
rfc9218
branch
4 times, most recently
from
a457f09 to
e1d5baf
Compare
arturobernalg
changed the title
@ok2c ACK — I’ll can own RFC 9218: client emission gated by |
| return new PriorityParams(null, null); | ||
| } | ||
|
|
||
| final CharArrayBuffer buf = new CharArrayBuffer(headerValue.length()); |
There was a problem hiding this comment.
@arturobernalg Please do not do that. There is no need to create a copy of the header value. The parser can work with any CharSequence. Can't you re-use MessageSupport parsing routines here?
Otherwise, looks good.
There was a problem hiding this comment.
@ok2c please do another pass
There was a problem hiding this comment.
@arturobernalg This feature is now your area of responsibility.
Add PRIORITY_UPDATE (0x10) and SETTINGS_NO_RFC7540_PRIORITIES (0x9). Client emits before HEADERS on opt-in; server accepts and applies. Wire into multiplexer + Priority header utils + tests.
@ok2c Understood. I’ll take ownership of the RFC 9218 prioritization feature and handle its maintenance going forward. |
|
@arturobernalg Please also add RFC 9218 to the protocol support section in |
@ok2c done |
| return streams.createActive(channel, streamHandler); | ||
| } | ||
|
|
||
| public final void sendPriorityUpdate(final int prioritizedStreamId, final PriorityValue value) throws IOException { |
There was a problem hiding this comment.
@arturobernalg By the way this method does not seem to be used anywhere. Is this intended?
There was a problem hiding this comment.
@ok2c Yes, that was intentional. The logic was inlined into the stream submit path, so the standalone helper became redundant. I'm going to removed it.
There was a problem hiding this comment.
@arturobernalg Please also remove H2RequestPriority interceptor for now. It is not being used anywhere in core and I presume your intention was to use it in client. In this case the interceptor should be in client.
There was a problem hiding this comment.
@arturobernalg Also PriorityFormatter is not being used anywhere in core. If they are not used in core they should not be in core. Re-submit them in client. Believe me you do not want to depend on a new release of core to be able to fix a bug in client.
There was a problem hiding this comment.
@arturobernalg Please let me know if you can live with #567. The functionality should be moved to client and we should put more work into the API design
|
@arturobernalg I did a sloppy job reviewing your code. It needs more work. |
@ok2c Can you point out the specific areas you find lacking, so I can focus the fixes where needed? I’ll take another pass, tighten the code paths, |
2 participants
This PR adds core support for RFC 9218: we advertise SETTINGS_NO_RFC7540_PRIORITIES=1, gate PRIORITY_UPDATE emission on the peer’s setting (or pre-SETTINGS), parse/accept PRIORITY_UPDATE frames in the base muxer, and introduce PriorityParamsParser for the Priority header (u/i members, tolerant parsing).