- From: Martin Thomson <martin.thomson@gmail.com>
- Date: Mon, 16 Jun 2014 16:28:43 -0700
- To: Mark Nottingham <mnot@mnot.net>
- Cc: HTTP Working Group <ietf-http-wg@w3.org>
This is overall quite reasonable feedback. >> From: "Poul-Henning Kamp" <phk@phk.freebsd.dk> >> Page 12: Reserved bits >> ---------------------- >> >> Historically RFCs have defined reserved bits opposite what would >> have been most useful later on approximately 50% of the time. >> >> I would split the difference: A R1 single-bit field, which must >> be sent as zero and ignored on reception, and a R2 single-bit field, >> which must be sent a zero, and where receiving a one is an error. Randomly? I think that we could probably make a few sensible decisions along these lines. Probably best to handle on a case-by-case basis though. For instance, the bits leading up to the length field in the frame header could be moved from R1 to R2 under this taxonomy and I'm happy to do that if others think that it's a good idea. https://github.com/http2/http2-spec/issues/531 >> Page 22ff: Priority >> ------------------- >> >> I think the PRIORITY stuff is going to be trouble and will need >> revisions later on to truly work. >> >> It will also be the first thing implementors skip in order to get >> something working fast, and they may never come back and implement >> it, unless serious numbers show it's worth the complexity. That seems to be an increasingly popular opinion. As of this moment, it's marked at risk and unless we get the serious numbers, I suspect that it will be removed in one way or another. >> Page 28ff: Padding >> ------------------ >> >> If padding is intended for obscuration of traffic patterns, it should >> be available in all frames to avoid "housekeeping" frames from leaking >> information about the relationships of other frames. >> >> For this reason I would make padding part of the framing format, >> rather than the individual message types. >> >> I'm also sceptical of demanding that padding be zeros, historically >> that has been a popular recipe for staring through compressions and >> encryptions. I would probably specify that it is up to the sender >> to decide what to pad with, with guidance to prefer PRN bits rather >> than predictable (ie: zeros) or meaningful data. The guidance I've gotten is split regarding this point. A known (zero or otherwise) padding allows for a weak cipher to be broken more easily. Random padding on the other hand leaves you further exposed to bad integrity checks. >> Page 28: Typographical >> ---------------------- >> >> It's a bad idea to end a sentence with "... %d." because of exactly >> what happens in draft 12: It might bet pushed to the next page >> and confuse everybody. s/1/one/ ? That text has been removed. >> Page 30 HEADERS/SEGMENTS >> ------- >> >> What does END_SEGMENT do on a HEADERS frame ? >> >> It looks like Copy&Paste error. https://github.com/http2/http2-spec/issues/397 >> If it's needed, no text explains what it does and it's missing on >> CONTINUATION. >> >> It's bogus that a lot of text separates HEADERS from CONTINUATION. It's not bogus, but it is perhaps poor document structure. >> It's also pretty non-obvious why we need CONTINUATION in the >> first place: Why not simply send HEADERS until one of them has >> the END_HEADERS bit set ? http://http2.github.io/faq/#why-the-rules-around-continuation-on-headers-frames >> Page 34: SETTINGS >> ------------------------ >> >> Are we absolutely certain that we will never need per-stream SETTINGS ? >> >> At least I would specify SETTINGS with non-zero stream to be ignored >> so that we can open that later. I think that the easiest way around this would be to define a new setting that allows for the use of SETTINGS on streams. If your peer sets that (on stream 0, of course), then you can start sending SETTINGS on streams. That is, if you ever discover a need. >> Page 34: SETTINGS_MAX_CONCURRENT_STREAMS >> ---------------------------------------- >> >> I would make the this field mandatory in the initial SETTINGS frames >> in order to make sure people think about the DoS potential. >> >> I also seriously doubt that anybody with a relevant DoS risk is >> going to open with a default value of "infinite" until they have >> seen some kind of "proof of work" on part of the client. >> >> I suspect such DoS mitigation will cause a lot of SETTINGS frames, >> but that is less prone to failure than trying to define a "slow-start" >> without any benefit of real-life DoS mitigation experience. >> >> (You should *seriously* consider trying to arrange an INTEROP with >> the specific goal of investigating DoS resistance.) >> >> Text should be added to explain what happens if N streams are >> open but (x < N) SETTINGS_MAX_CONCURRENT_STREAMS is sent (the >> already open streams survive, new streams must wait for them to >> finish ?) We already have something like that for flow control: http://http2.github.io/http2-spec/index.html#rfc.section.6.9.3 I think that allowing the endpoint choose is best. I'll add text for that. >> What happens of SETTINGS_MAX_CONCURRENT_STREAMS gets set to zero >> and stays there ? How long time does the client wait for before >> opening a new connection ? Fact is, zero isn't really all that special. So the logic is the same. Clients that want to use new streams, but can't are going to have to come to some conclusion on their own. Basically, it all boils down to how long a client is willing to wait before giving up on that connection and making a new one. >> Are clients allowed to open multiple connections to bypass the >> SETTINGS_MAX_CONCURRENT_STREAMS limit ? >> >> Are proxies ? That is a good point. https://github.com/http2/http2-spec/issues/529 I think that both come down to providing advice around concurrency limits. We need to work out what the messaging here is. I don't think that we want to encourage the use of bypass mechanisms. >> Page 36: SETTINGS synchronization >> --------------------------------- >> >> This is utterly bogus. >> >> There is no way a SETTINGS frame can overtake any other frame while >> in transit, and there is no way the receiver can refuse the SETTINGS. >> >> There is also none of the settings which require synchronization between >> the ends or which the sender cannot prepare for before sending the >> SETTINGS frame. >> >> Simply demand that SETTINGS be applied atomically when received, before >> processing any subsequent frames and we're done. >> >> If the sender *really* wants to know that the SETTINGS has been >> registered in the far end, it can send a PING after the SETTINGS >> and wait for the PING+ACK. >> >> As it is now, SETTINGS causes the loss of an RTT if implemented as >> specified. That's not true. You send SETTINGS, you carry on. You only lose RTTs if you want to apply tight limits and then open them again. As for the rest, yes, this isn't 100% necessary, but it does give an endpoint the ability to say at what point a setting should have been respected. Thus, if I set SETTINGS_MAX_CONCURRENT_STREAMS to 0, then I know that I can kill the connection when a new stream arrives. And the explicit acknowledgement allows for different implementation strategies. We've already seen that people are enqueuing packets well ahead of what the underlying transport permits. (That might be suboptimal in some respects, but I'm sure that these people know what they are doing.) >> >> Page 39: GOAWAY >> --------------- >> >> I would give the sender the ability to add a small-ish integer >> to the Last-Stream-ID, as a way to avoid wasted work. >> >> Doing that, I think an opening gambit of >> >> SETTINGS(MAX_CONNCURRENT_STREAMS = n) >> GOAWAY(Last-Stream-ID = n) >> >> could be a very efficient DoS mitigation strategy, which would give >> legitimate users a chance. In particular if we allow GOWAY to >> update the Last-Stream-ID once the client has proven non-hostile. >> >> In fact, it's not obvious to me why GOAWAY isn't simply a field >> in SETTINGS to begin with ? Except that GOAWAY explicitly says that stream n+2 is safe to retry elsewhere. How would your proposal here avoid issues with that? >> Page 42: Flow Control Window >> ----------------------------- >> >> Again: Why is WINDOW_UPDATE a separate frame type ? a field >> in SETTINGS would do just fine. Because you need to be able to advertise increased space. And because we decided not to open up the synchronization issues that come with a rate-based or absolute window size advertisement scheme. >> For obvious DoS reasons, it's a mistake to not apply flowcontrol >> to HEADERS. Yes, you can't use flow control as a tool to mitigate DoS that uses HEADERS. I think that we've reached the conclusion that we don't want to be using that hammer to mitigate all similar problems. I know that's not a particularly satisfactory answer, but there are a bunch of secondary concerns. In other words, it was not as obvious as you might like to think. >> I would allocate frame Type values so that the top bit indicates >> "subject to flow-control" to simplify implementation. Similarly >> expending a bit to mark hop-to-hop frames would make life easier >> for high performance implementations. SPDY effectively did that. Given the status quo, I'm not sure that (!type) and (type&mask) are measurably different performance-wise. >> I also think it is a mistake to exempt the 8 byte frame header from >> the FCW: You can fill a lot of pipe with 1 byte DATA frames when >> the overhead isn't counted. This is another place where the flow control window isn't usable. It doesn't bother me a great deal. It seems like there are bigger holes in this fence. >> The explanation about SETTINGS_INITIAL_WINDOW_SIZE causing negative >> window-sizes is somewhat clumsy because some unrealted stuff breaks >> the flow. Maybe move "A SETTINGS frame cannot alter the connection >> flow control window." and "An endpoint MUST ... FLOW_CONTROL_ERROR" >> out of the way. Done. >> Page 49: BLOCKED Removed already. >> Page 50: Error messages >> ----------------------- >> >> The RFC should propose text-messages for presentation purposes I'm sure that people would welcome a contribution in this regard. >> Page 51: out of order flags >> --------------------------- Yes, we know. It's probably not worth fixing though. >> Page 51: More details in example >> -------------------------------- >> >> The example in 8.1 should show where the END_HEADERS and END_STREAM bits >> are set. It does. >> Page 51: To strict ordering ? >> ------------------------------ >> >> Other frames (from any stream) MUST NOT occur between either HEADERS >> frame and the following CONTINUATION frames (if present), nor between >> CONTINUATION frames. >> >> Isn't this needlessly strict ? No harm would come from DATA frames >> or SETTING frames being stuffed in there. >> >> All this trouble could be avoided by only submitting headers for >> decompression, as a unit, when the END_HEADERS have been received. That creates a nice state exhaustion/denial of service opportunity that we decided not to permit. Especially because header compression causes the loss of a connection. >> Page 51: Gibberish >> ------------------ >> >> This needs to be translated to human readable form: >> >> Otherwise, frames MAY be interspersed on the stream between these >> frames, but those frames do not carry HTTP semantics. In particular, >> HEADERS frames (and any CONTINUATION frames that follow) other than >> the first and optional last frames in this sequence do not carry HTTP >> semantics. Improved text gratefully accepted. >> Page 51: Trailing headers >> ------------------------- >> >> Support for trailing headers should have a bit in SETTINGS. I guess that we could do it. https://github.com/http2/http2-spec/issues/530 Not sure if it will fly, but we can see. Q: >> How dow we know the DATA section is done if there are trailing headers ? A: >> By appearance of the first (trailer-)HEADER ? >> >> Trailing headers should be marked in a way that does not force a >> high-perf load-balancer to decode all headers to find out. Setting >> a flag bit in the first block of HEADERS/CONTINUATION is free for >> the sender (they have to find out at that time anyway) and saves a >> lot of time for the receiver. It's easy, HEADERS w/ END_STREAM is worth looking at. >> Page 55: reverse order >> ---------------------- >> >> It would make more sense if the ":" header fields were described >> before they appear in the examples on the preceeding pages. Already done. Thanks. >> page 57: query part >> ------------------- >> >> The query part of the URI should get its own ":query" psuedo-field >> for reasons of othogonality and to reduce the amount of data typical >> load-balancers have to examine. I think that we discussed this and decided against it. Happy to follow up, but I'd have thought that the gains would be marginal. >> Page 59: cookie compression needs example >> ----------------------------------------- >> >> Basic confusion-prevention. I'll look into it. >> Page 59: Clarification >> ---------------------- >> >> [...]the sum of the uncompressed DATA frame payload lengths [...] >> >> Should probably (better) clarify that 'uncompressed' obny pertains to the >> per frame GZIP compression. Removed. >> I havn't looked at the PUSH stuff, I still consider it a fundamentally >> bad idea. Noted.
Received on Monday, 16 June 2014 23:29:12 UTC