-
Notifications
You must be signed in to change notification settings - Fork 204
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
Limit pushes using MAX_PUSH_ID #711
Conversation
draft-ietf-quic-http.md
Outdated
The MAX_PUSH_ID frame (type=0xA) is used by clients to control the number of | ||
server pushes that the server can initiate. This sets the maximum value for a | ||
Push ID that the server can use in a PUSH_PROMISE frame. Consequently, this | ||
also limits the number of push streams that the server can initiate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm following this completely. Clearly MAX_PUSH_ID limits the number of promises that the server can make. But isn't the number of actual push streams limited by the MAX_STREAM_ID?
(This makes me wonder if "Promise ID" might be a better name than "Push ID" but I may well just be confused.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can't deliver a push that hasn't been promised, so the actual limit is whichever is smaller.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. (Though I think it common practice we expect the promise limit to be higher than the stream limit so we would not expect this to be the lower of the two.) Perhaps the text could say:
Consequently, this also limits the number of push streams that the server can initiate in conjunction with MAX_STREAM_ID.
(or some such)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally good; minor nits other than the frame ID.
draft-ietf-quic-http.md
Outdated
of type HTTP_MALFORMED_PUSH. | ||
|
||
A server SHOULD use Push IDs sequentially, starting at 0. A client uses the | ||
MAX_PUSH_ID frame ({{frame-max-push-id}}) and SETTINGS_MAX_PUSH_ID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I'd mention these in the order they're used; the setting to provide an initial value and the frame to increment it later.
draft-ietf-quic-http.md
Outdated
MAX_PUSH_ID frame ({{frame-max-push-id}}) or the SETTINGS_MAX_PUSH_ID setting | ||
({{settings-parameters}}). A client that receives a PUSH_PROMISE that contains | ||
a larger Push ID than is allowed MUST be treated as a connection error of type | ||
HTTP_MALFORMED_PUSH_PROMISE. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"is allowed" makes it sound like an absolute limit; might be worth rewording slightly.
draft-ietf-quic-http.md
Outdated
@@ -794,6 +815,37 @@ the server MAY send another GOAWAY frame with an updated last stream identifier. | |||
This ensures that a connection can be cleanly shut down without losing requests. | |||
|
|||
|
|||
# MAX_PUSH_ID {#frame-max-push-id} | |||
|
|||
The MAX_PUSH_ID frame (type=0xA) is used by clients to control the number of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that you're now conflicting with the frame type for the ALTSVC frame in the HTTP/2 registry. ORIGIN is using 0xC, which suggests someone has 0xB but I can't think of who off the top of my head right now. Might want to jump out to at least 0xD to be safe, or (ab)use 0x8 instead.
for a Push ID that the server can use (see {{frame-push-promise}}). A | ||
MAX_PUSH_ID frame cannot reduce the maximum Push ID; receipt of a MAX_PUSH_ID | ||
that contains a smaller value than previously received MUST be treated as a | ||
connection error of type HTTP_MALFORMED_MAX_PUSH_ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
General comment: Because these are on the control stream, which means guaranteed in-order delivery, we could use the HTTP/2 model of sending an increment rather than an absolute number. I'm not sold that the few bytes saved are worth it, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we can make a bigger saving here by not sending the frame as often, noting that MAX_STREAM_ID covers the resource exhaustion issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the savings are worth it, since this shouldn't be sent that often really as it is.
On resource exhaustion, I think both MAX_STREAM_ID and MAX_PUSH_ID both cover that issue, just at different levels. QUIC decides MAX_STREAM_ID based on its resource constraints for streams, and the HTTP "layer" decides MAX_PUSH_ID based on its constraints for handling pushed resources. I think these are independent.
draft-ietf-quic-http.md
Outdated
HTTP_SETTINGS_ON_WRONG_STREAM (0x0F): | ||
: A SETTINGS frame was received on a request control stream. | ||
HTTP_WRONG_STREAM (0x0F): | ||
: A frame was received on a different stream to where it is permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"different... to" is British; "different... than" is American. "Different... from" is neutral. Maybe we should use that, or just sidestep the issue with "on a stream where it is not permitted."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learn something every day. I say that in its most Lovecraftian meaning, of course.
draft-ietf-quic-http.md
Outdated
A server SHOULD use Push IDs sequentially, starting at 0. A client uses the | ||
SETTINGS_MAX_PUSH_ID ({{settings-parameters}}) setting and the MAX_PUSH_ID frame | ||
({{frame-max-push-id}}) to limit the number of pushes that a server can promise | ||
initiate. A client MUST treat receipt of a push stream with a Push ID that is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"promise initiate" needs to have only one word or a conjunction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Martin! Looks great. A few comments.
for a Push ID that the server can use (see {{frame-push-promise}}). A | ||
MAX_PUSH_ID frame cannot reduce the maximum Push ID; receipt of a MAX_PUSH_ID | ||
that contains a smaller value than previously received MUST be treated as a | ||
connection error of type HTTP_MALFORMED_MAX_PUSH_ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the savings are worth it, since this shouldn't be sent that often really as it is.
On resource exhaustion, I think both MAX_STREAM_ID and MAX_PUSH_ID both cover that issue, just at different levels. QUIC decides MAX_STREAM_ID based on its resource constraints for streams, and the HTTP "layer" decides MAX_PUSH_ID based on its constraints for handling pushed resources. I think these are independent.
draft-ietf-quic-http.md
Outdated
HTTP_SETTINGS_ON_WRONG_STREAM (0x0F): | ||
: A SETTINGS frame was received on a request control stream. | ||
HTTP_WRONG_STREAM (0x0F): | ||
: A frame was received on a different stream from where it is permitted. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from -> than ? I mostly don't care because it might be about local idioms, but it took me a moment to parse :-)
Offline, Ryan suggested "A frame was received on stream where it is not permitted", which SGTM as well (though it is semantically slightly different).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
draft-ietf-quic-http.md
Outdated
maximum Push ID that a server can use. This setting only has meaning when | ||
set by a client; a server MUST NOT provide a value for this setting. A | ||
client MUST treat receipt of this setting from a server as a connection | ||
error of type HTTP_MALFORMED_SETTINGS. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be 0? I assume so, since there's no other way to disable PUSH now. Can you state this explicitly here (that a client can disable push by setting this value to 0)?
server pushes that the server can initiate. This sets the maximum value for a | ||
Push ID that the server can use in a PUSH_PROMISE frame. Consequently, this | ||
also limits the number of push streams that the server can initiate in addition | ||
to the limit set by the QUIC MAX_STREAM_ID frame. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems silly but possible to send this with 0. Can you add a sentence that says this MUST be > 0?
The MAX_PUSH_ID frame carries a 32-bit Push ID that identifies the maximum value | ||
for a Push ID that the server can use (see {{frame-push-promise}}). A | ||
MAX_PUSH_ID frame cannot reduce the maximum Push ID; receipt of a MAX_PUSH_ID | ||
that contains a smaller value than previously received MUST be treated as a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would explicitly state that this previous value may have been received via MAX_PUSH_ID or SETTINGS_MAX_PUSH_ID. Perhaps "a smaller value than that received via a previous MAX_PUSH_ID or SETTINGS_MAX_PUSH_ID ..." ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sending a frame with a value less than you provided in transport settings is a clear error, actually, since you know the transport settings necessarily came before the frame.
b6bcd43
to
95ff370
Compare
HTTP_MALFORMED_DATA description mentioned HEADERS and it wasn't in the IANA table. HTTP_WRONG_STREAM was defined as HTTP_SETTINGS_ON_WRONG_STREAM.
This repurposes SETTINGS_ENABLE_PUSH as the initial limit. A client can use this to limit the number of outstanding pushes. I've included advice on increasing the limit, not as the pushes are first used, but as the pushes are completed (or cancelled). There wasn't a rule for handling a truncated push stream header, so I added that as well. The HTTP_DUPLICATE_PUSH error was reclaimed for signaling all three types of error with a push stream header: truncation, duplicate Push ID, and exceeding MAX_PUSH_ID.
95ff370
to
b38ae53
Compare
Heh -- thanks for rephrasing, it's one of those trivial yet annoying things.
…On Wed, Aug 9, 2017 at 5:46 PM, Martin Thomson ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In draft-ietf-quic-http.md
<#711 (comment)>:
> @@ -857,14 +910,17 @@ HTTP_INTERRUPTED_HEADERS (0x0E):
: A HEADERS frame without the End Header Block flag was followed by a frame
other than HEADERS.
-HTTP_SETTINGS_ON_WRONG_STREAM (0x0F):
-: A SETTINGS frame was received on a request control stream.
+HTTP_WRONG_STREAM (0x0F):
+: A frame was received on a different stream from where it is permitted.
I took Ryan's text, but I moved that to #712
<#712>. This PR will appear to
be include the change, but that's just because I rebased on top of #712
<#712> on the assumption that
it will be merged.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#711 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AKjg1CBuuPYrMA7q8eURZwd82JARTWNcks5sWlLTgaJpZM4OwaxD>
.
|
This repurposes SETTINGS_ENABLE_PUSH as the initial limit.
A client can use this to limit the number of outstanding pushes. I've included
advice on increasing the limit, not as the pushes are first used, but as the
pushes are completed (or cancelled).
There wasn't a rule for handling a truncated push stream header, so I added
that as well. The HTTP_DUPLICATE_PUSH error was reclaimed for signaling all
three types of error with a push stream header: truncation, duplicate Push ID,
and exceeding MAX_PUSH_ID.
Closes #709.