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

PRIORITY_UPDATE before headers #1722

Closed
martinthomson opened this issue Oct 6, 2021 · 8 comments
Closed

PRIORITY_UPDATE before headers #1722

martinthomson opened this issue Oct 6, 2021 · 8 comments

Comments

@martinthomson
Copy link
Contributor

martinthomson commented Oct 6, 2021

To solve this condition, for the purposes of scheduling, the most recently received PRIORITY_UPDATE frame can be considered as the most up-to-date information that overrides any other signal.

This is not necessarily obvious, but the implication here is that PRIORITY_UPDATE is only ever sent after the header block. That and the only reason PRIORITY_UPDATE might be received before the header block is reordering. I think that it would be worth saying that here.

This part is problematic in light of that:

A client MAY send a PRIORITY_UPDATE frame before the stream that it references is open [...]

I see no reason to allow that. In HTTP/2, this only adds complexity.

In HTTP/3, the receiver might need to deal with reordering, but this formulation might result in some interesting corner cases. What if a client had MAX_STREAMS at 2 (0 and 4 are the only request streams). What if PRIORITY_UPDATE referenced stream 8? That is the next request, but is it permissible to prioritize it?

Edit: I missed the fancy rules that exist for these. Obviously this has been thought through carefully. However, I still think that it is unnecessary and overly complex.

If a frame can only be sent after the thing that is being prioritized has been created (locally), then the only thing that needs to be considered is reordering. In HTTP/2, that's a non-issue, so there is no need for buffering. In HTTP/3, there is some buffering, but that can be bounded by stream/request/push ID limits.

@MikeBishop
Copy link
Contributor

You're right, the rules were designed in the context of H3 and just translated into H2 because sending early looks the same as reordering from the receiver side. You can't send a PRIORITY_UPDATE frame for a stream you couldn't open, and the server doesn't have to keep more priorities than it would be willing to keep open streams.

If we prohibited sending the frame before the stream had been created, the H2 rules could be a lot simpler. I think the rules as written are probably correct for H3, but they're already simpler because H3 was built around reordering.

@LPardue
Copy link
Contributor

LPardue commented Oct 7, 2021

I don't see much problem, HTTP/2's PRIORITY frame behaves similarly "A PRIORITY frame can be sent in any stream state, including idle or closed streams.". We don't need the closed stream aspect since we don't have the problem of dependencies, so things are already simpler here. There's a reasonable argument that we don't need the prioritization of idle streams (the famous Firefox approach).

ISTM that preventing the frame before HEADERS needs implemenattions to do something, and the difference between doing that or this is marginal. There is also a danger of presuming implementation process frames on request streams and on stream 0 strictly in the order they arrive, I don't have evidence this is the case

From a purest view, having a consistent behaviour of using the the frame across H2 and H3 is attractive. Alas, if the current text is really an issue for H2 implementers I am open to being convinced of a need for change

@kazuho
Copy link
Contributor

kazuho commented Oct 7, 2021

IIRC, for HTTP/2, we made the deliberate choice to allow clients send PRIORITY_UPDATE frames before HEADERS.

The reason is that clients cannot send two frames at once. Either of the two would be sent before the other. If we prohibit clients from sending PRIORITY_UPDATE before HEADERS, then the client has to send PRIORITY_UPDATE after HEADERS. Doing so means that there's chance that the server might receive HEADERS, process the request using the default priority due to lack of the Priority header field in the request, then receive PRIORITY_UPDATE.

That would be an unexpected prioritization error to clients that use PRIORITY_UPDATE frames for signaling initial priority.

Hence we have current design, and I think it makes sense.

@martinthomson
Copy link
Contributor Author

I wouldn't use h2 priorities as a template here. Part of the reason for this spec is to avoid replicating mistakes and having priority state that exists independent of stream state is one of those mistakes.

@kazuho would have a hop-by-hop override available before a stream is created. I don't think that justifies all the complexity that goes with that, but I'm open to hearing from other implementers about that.

@LPardue
Copy link
Contributor

LPardue commented Oct 7, 2021

I wouldn't use h2 priorities as a template here. Part of the reason for this spec is to avoid replicating mistakes and having priority state that exists independent of stream state is one of those mistakes.

There is no need to build priority state based on PRIORITY_UDPATE before HEADERS. An implementation can hold stateless frames and only apply them once the corresponding request arrives. That's not totally free but the commitment is limited by the text (buffer the last frame, commitment limited by concurrency thresholds)

@LPardue
Copy link
Contributor

LPardue commented Oct 18, 2021

I'm not seeing a compelling reason to change the design aspect just for H2. @martinthomson are you willing to die on this hill? We can take it to the list for more input if needs be.

@martinthomson
Copy link
Contributor Author

No. It's complexity, but justified.

@LPardue LPardue added discuss A candidate for discussion at a meeting and removed discuss A candidate for discussion at a meeting labels Oct 19, 2021
@LPardue
Copy link
Contributor

LPardue commented Oct 19, 2021

Thanks, closing this then.

@LPardue LPardue closed this as completed Oct 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

4 participants