-
Notifications
You must be signed in to change notification settings - Fork 203
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
Import most H2 references #3407
Conversation
494eca8
to
c1a16dd
Compare
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. A few corner cases seem problematic though.
carrying a request which is not cacheable, is not known to be safe, or that | ||
indicates the presence of a request body. If the pushed response arrives on a | ||
push stream, this MAY be treated as a stream error of type | ||
H3_STREAM_CREATION_ERROR. |
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.
This is a bit weird in the sense that some of this is just an error. If I get a PUSH_PROMISE with Content-Length > 0
, why would I cancel it rather than treat that as an error?
Whether something is "cacheable" might be unknowable based on the request alone, but some errors are pretty sharp. There are other cases which are less clear too, such as Content-Type
maybe indicating the presence of a request body, so I get how this might be tricky to handle in the general case.
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.
Requests can be cacheable, but have answers which turn out not to be cacheable. The distinction we settled on for H2 was that you can only promise cacheable requests, but the response need not be cacheable.
Yes, all of this is an error. The question is whether:
- it needs to be a stream error on the response that contains the push
- it can be detected at the right layer for that to happen
My reasoning is primarily that, if you need to decompress and examine the headers to determine whether something is in error, an immediate stream error is not the right response to mandate. Instead, you mark the push as invalid -- cancel it, and reject the push stream if it purports to arrive.
Really, this is a case of the push being malformed, and the idea is to define a response to malformed pushes that's commensurate with malformed responses -- don't use it.
draft-ietf-quic-http.md
Outdated
|
||
A CONNECT request that does not conform to these restrictions is malformed (see | ||
{{malformed}}). The request stream MUST NOT be closed at the end of the | ||
request. |
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.
This last sentence can go. It is entirely valid for the request stream to be closed without sending anything, though it is admittedly not always very useful.
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 it's useful as an exception to the more general requirement that the stream get closed at the end of the request. But yes, we can reword it to something like "MAY not be closed." (I would say "need not be closed," except that an exception to a MUST elsewhere feels like it needs its own normative language.)
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 for writing this.
d5749e4
to
5983e50
Compare
The intent is that RFC7540 is an informative reference after this PR and #3352. The combination fixes #3264, modulo Security Considerations.
It's intended to be purely editorial, despite the normative language -- these are requirements which were previously incorporated by reference to RFC 7540 and are now explicitly stated in the document. However, some of the imported requirements didn't have specifications about what you do if they're not met, so I did have to add that text.
The Security Considerations of RFC7540 are substantial. If we think it's worth not simply referencing 7540 for that discussion, that deserves its own PR. Opinions on the value of that are welcome here, however.
@royfielding, GitHub doesn't let me directly request your review, but I would appreciate it.