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

The server doesn't override priority #1720

Closed
wants to merge 2 commits into from
Closed

The server doesn't override priority #1720

wants to merge 2 commits into from

Conversation

martinthomson
Copy link
Contributor

The server only informs the client of what it did. This also informs intermediaries, who might
factor that into how they operate.

I tweaked the next sentence here for grammar, though I think that sentence needs a little work too.

The server only informs the client of what it did.  This also informs intermediaries, who might
factor that into how they operate.

I tweaked the next sentence here for grammar, though I think that sentence needs a little work too.
Co-authored-by: Mike Bishop <mbishop@evequefou.be>
@@ -376,8 +376,8 @@ where to send registration requests.

The Priority HTTP header field can appear in requests and responses. A client
uses it to specify the priority of the response. A server uses it to inform
the client that the priority was overwritten. An intermediary can use the
Priority information from client requests and server responses to correct or
the client and intemediaries of the priority that was applied. An intermediary can use
Copy link
Contributor

Choose a reason for hiding this comment

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

a server behind an intermediary is not applying priority. That is addressed by the next sentence but in isolation this sentence can be untrue.

Copy link
Contributor

Choose a reason for hiding this comment

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

and actually, omission of priority in responses is also a signal that the requested priority was ok. The intent of the response header is to change (override) only the priority parameters that the server disagrees with.

Copy link
Contributor

Choose a reason for hiding this comment

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

How is a server not applying priority? It has to prioritize responses to the intermediary, after all. Or are you saying that the Priority header is really focused on the client-to-first-intermediary link, and the response is specifically focused on the server's suggested behavior on that link?

Copy link
Contributor

Choose a reason for hiding this comment

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

What does it mean to apply the priority?

If we take the view that the signals are a hint, then one hint is overridding the other. How that manifests in actual prioritization or scheduling is quite obtuse. What a server (or servers) behind an intermediary do here is gonig to be complex.

Take for example the case where an intermediary splits concurrent H3 request to different HTTP/1.1 backends that only serve requests serially as fast as they can. In such a case the servers can't do action in isolation. But a consistent stateless strategy for providing a Priority header back to the intermediary can do a good job of nudging the intermediary to schedule things in the right way.

Ignoring intermediaris, the story is much more simple. But oversimplifying the statement here might mislead people of the full intended design.

Copy link
Contributor

Choose a reason for hiding this comment

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

Over HTTP/1.1, the server can't prioritize anything locally on the connection, that's true. But the back-end connection need not be H1, and I think we set out to have this mechanism be for all HTTP versions, with version-specific optimizations where reasonable. Generically, the client makes a statement of its desired priority; the server makes a statement contributing what it thinks priority ought to be; each hop on the response chain (the server and any intermediaries) will consider both of those inputs on their return hop.

Copy link
Contributor

Choose a reason for hiding this comment

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

right I was using a single example. There's lots of different ways. The intent was, like you said earlier, that the focus is on "client-to-first-intermediary link" and on response bytes getting served. It's just in the no intermediary case, that doesn't exist.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, then both the old and the proposed text are incorrect, though I understand the old text better. How about....

Suggested change
the client and intemediaries of the priority that was applied. An intermediary can use
a potential intermediary of the priority that should be applied. An intermediary can use

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks Mike. That's better but it didn't quite do it for me. I applied some more drastic surgery in #1731 which I believe address the specific point here by just pointing people to the text that explains things in detail.

LPardue added a commit that referenced this pull request Oct 7, 2021
Fixes #1716 and #1720.

We define priority parameters as a structured fields dictionary. But
don't explicitly come out and say that the Priority header fields should
be treated that way. So this change fixes that.

The text in the Priority header section attempts to give a concise
overview of how it is used but that caused confusion. Since the text
just summarised information presented elsewhere, this change refactors
things to point to those sections.

While we are tweaking the priority header section, we can pull the text
about parsing failure handling up into the priority parameters section
so that there is a single place that gathers parsing considerations.

Finally, I noticed that our header registration was a bit old fashioned
in light of what's happening in HTTP core. So I updated that too.
LPardue added a commit that referenced this pull request Oct 15, 2021
* refactor priority parameters and Priority header bits

Fixes #1716 and closes PR #1720.

We define priority parameters as a structured fields dictionary. But
don't explicitly come out and say that the Priority header fields should
be treated that way. So this change fixes that.

The text in the Priority header section attempts to give a concise
overview of how it is used but that caused confusion. Since the text
just summarised information presented elsewhere, this change refactors
things to point to those sections.

While we are tweaking the priority header section, we can pull the text
about parsing failure handling up into the priority parameters section
so that there is a single place that gathers parsing considerations.

Finally, I noticed that our header registration was a bit old fashioned
in light of what's happening in HTTP core. So I updated that too.

Co-authored-by: Kazuho Oku <kazuhooku@gmail.com>
Co-authored-by: Martin Thomson <martin.thomson@gmail.com>
@LPardue
Copy link
Contributor

LPardue commented Oct 15, 2021

The alternative PR #1731 was taken over this

@mnot mnot deleted the as-applied branch November 17, 2021 01:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

3 participants