Subject: AD review of draft-ietf-httpbis-http2bis-05

From: Francesca Palombini <[email-masked]>
Date: Wed, 10 Nov 2021 21:23:37 +0000

Thank you very much to the authors and the wg for another well written document.

I don't have major comments. I have some minor comments, and a couple of nits or request for clarifications, which you can address together with any other last call comments. The requests for adding references to specific sections are there to hopefully make it easier for a reader to navigate the different specifications referenced, I obviously think they would improve readability but feel free to take or leave as you please.

Additionally, before starting the last call, I want to bring your and the wg's attention to the first 2 points, related to references.

I have opened a github issue with the text below: https://github.com/httpwg/http2-spec/issues/974.
Francesca

1. -----

FP: Is the working group aware that RFC 793bis is in IESG evaluation (https://datatracker.ietf.org/doc/draft-ietf-tcpm-rfc793bis/ ) ? Was the choice of having a normative reference to 793 conscious, in order to avoid any delay that might come from publication of draft-ietf-tcpm-rfc793bis? (Just checking this was considered)

2. -----

   for HTTP/2 over TLS.  The general TLS usage guidance in [TLSBCP]
   SHOULD be followed, with some additional restrictions that are
   specific to HTTP/2.

FP: Given this requirement, I would have expected to see [TLSBCP] normatively referenced, rather than informatively.

3. -----

   layer.  The frame and stream layers are tailored to the needs of the
   HTTP protocol and server push.

FP: I would think the server push is part of the HTTP protocol, which makes this formulation "HTTP protocol and server push" confusing.

4. -----

   Type:  The 8-bit type of the frame.  The frame type determines the

   Flags:  An 8-bit field reserved for boolean flags specific to the

FP: I would find a reference to the IANA registry useful.

5. -----

   implementation of flow control can be difficult.  When using flow
   control, the receiver MUST read from the TCP receive buffer in a
   timely fashion.  Failure to do so could lead to a deadlock when

FP: "When using flow control" might be rephrased to indicate "when flow control limits are lower than the maximum" (or something of the sort), since if I understand correctly the capability is always used, it is just the window size that changes (effectively implementing control of flow). Also "timely fashion" - I know that it is probably hard, but it would be nice to have a more precise qualification, or at least a hint of what is considered timely.

6. -----

   stream that it successfully received from its peer.  The GOAWAY frame
   includes an error code that indicates why the connection is

FP: "error code" - here as well I would have liked a reference to the IANA registry.

7. -----

   Exclusive:  A single-bit flag.  This field is only present if the
      PRIORITY flag is set.

   Stream Dependency:  A 31-bit stream identifier.  This field is only
      present if the PRIORITY flag is set.

   Weight:  An unsigned 8-bit integer.  This field is only present if
      the PRIORITY flag is set.

FP: I would have expected to see some definition of how the fields are used. If this is defined somewhere else, a reference would be good.

8. -----

   SETTINGS Frame {
     Length (24),
     Type (8) = 4,

     Unused Flags (7),
     ACK Flag (1),

     Reserved (1),
     Stream Identifier (31),

     Setting (48) ...,
   }

   Setting {
     Identifier (16),
     Value (32),
   }

FP: Is there any reason why the Stream Identifier line is not:
     Stream Identifier (31) = 0,

9. -----

      a server does include a value it MUST be 0.  A client MUST treat
      receipt of a SETTINGS frame with SETTINGS_ENABLE_PUSH set to 1 as
      a connection error (Section 5.4.1) of type PROTOCOL_ERROR.

FP: This is just my curiosity: what is the reason for this stronger requirement - I would think it shouldn't be a problem for the sender if it wants to advertise that it would permit/support server push. What am I missing?

10. -----

      A value of 0 for SETTINGS_MAX_CONCURRENT_STREAMS SHOULD NOT be
      treated as special by endpoints.  A zero value does prevent the

FP: When is it ok that the 0 value is treated as special?

11. -----

   set.  Upon receiving a SETTINGS frame with the ACK flag set, the
   sender of the altered settings can rely on the value having been
   applied.

FP: nit s/value/values. Also I believe this could be misunderstood - can it be made more precise on the fact that only the values that are present in the received frame with the ACK flag set (and not those that might have been ignored because not understood) have been applied.

12. -----

  A receiver MUST treat the receipt of a WINDOW_UPDATE frame with an
   flow-control window increment of 0 as a stream error (Section 5.4.2)

FP: nit s/an/a

13. -----

   FLOW_CONTROL_ERROR (0x3):  The endpoint detected that its peer
      violated the flow-control protocol.

   STREAM_CLOSED (0x5):  The endpoint received a frame after a stream
      was half-closed.

FP: would be good to add a reference to the relevant sections.

14. -----

   set after receiving the HEADERS frame that opens a request or after
   receiving a final (non-informational) status code MUST treat the

FP: Where is a "non-informational status code" defined?

15. -----

FP: Are the following two sentences in Section 8.1.1. in contradiction?

   request or response.  Malformed requests or responses that are
   detected MUST be treated as a stream error (Section 5.4.2) of type
   PROTOCOL_ERROR.

   on the remainder of the request being correct.  A server or
   intermediary MAY use RST_STREAM -- with a code other than
   REFUSED_STREAM -- to abort a stream if a malformed request or
   response is received.

FP: In section 5.4.2. I read:

   An endpoint that detects a stream error sends a RST_STREAM frame

So the first sentence above implies RST_STREAM MUST be sent, while the second sentence states RST_STREAM MAY be sent.

16. -----

   their definitions in Sections Section 5.1 of [5.1] and Section 5.5 of
   [5.5] of [HTTP] respectively and treat messages that contain

FP: References need fixing.

17. -----

     from the control data of the original request, unless the the

FP: nit Remove one "the"

18. -----

      Note that request targets for CONNECT or asterisk-form OPTIONS
      requests never include authority information.

FP: Please add a reference to 7.1 of [HTTP] as this is the first time "asterisk-form OPTION" appear in this document.

19. -----

   Advertising a SETTINGS_MAX_CONCURRENT_STREAMS value of zero disables
   server push by preventing the server from creating the necessary
   streams.  This does not prohibit a server from sending PUSH_PROMISE
   frames; clients need to reset any promised streams that are not
   wanted.

FP: Do these two sentences contradict each other? In the first sentence I am reading that the server can't create the necessary stream to send the PUSH_PROMISE frame, in the second sentence I read that it can?

20. -----

   on a DATA frame is treated as being equivalent to the TCP FIN bit.  A

FP: Can a reference be added to the section where the TCP FIN bit is defined?

21. -----

     Content-Type: image/jpeg   ==>     - END_STREAM
     Content-Length: 123                + END_HEADERS

FP: I think it would be good to add a sentence about the meaning of - and + (which I understand to be flag set or not set) in section 2.2.

22. -----

   treated as delimiters in other HTTP versions.  An intermediary that
   translates an HTTP/2 request or response MUST validate fields
   according to the rules in Section 8.2 roles before translating a
   message to another HTTP version.  Translating a field that includes

FP: is "roles" supposed to be there?

23. -----

   The CONNECT method can be used to create disproportionate load on an
   proxy, since stream creation is relatively inexpensive when compared

FP: nit s/an/a

24. -----

FP: I think it would be good to keep the 3rd paragraph in Section 11 instead of asking the RFC Editor to remove it, just to keep a trace of the registries that have been defined in 7540, since those registries will now reference this document, but this document does not contain all the definitions of the different fields.

25. ----

   Comments:  Obsolete; see Section 11.1

FP: I would suggest to be explicit and add "of this document" (unless links can be maintained in IANA registries Comments fields).

---

From: Cory Benfield <[email-masked]>
Date: Thu, 11 Nov 2021 10:05:02 +0000

Thanks for the detailed review Francesca. A few responses inline.

> FP: This is just my curiosity: what is the reason for this stronger requirement - I would think it shouldn't be a problem for the sender if it wants to advertise that it would permit/support server…

This MUST applies only to clients receiving the setting. If the client
received a SETTINGS frame with SETTINGS_ENABLE_PUSH set to 1, the
server would be advertising support for receiving pushes. This
document forbids servers from receiving pushes: PUSH_PROMISE frames
may only be sent to clients, they contain requests and not responses,
and so on. As a result, this document forbids servers from claiming
they support receiving pushes.

> FP: Do these two sentences contradict each other? In the first sentence I am reading that the server can't create the necessary stream to send the PUSH_PROMISE frame, in the second sentence I read…

Sadly no. § 5.1.2 says "Streams in the reserved state do not count
toward the stream limit". Setting SETTINGS_MAX_CONCURRENT_STREAMS to 0
as a client prevents the server from actually _opening_ streams, but
sending PUSH_PROMISE frames doesn't open them, it merely reserves
them. As a result it functionally disables server push, but it does
not forbid the server from sending the frames, it just prevents them
being useful.

---

From: Cory Benfield <[email-masked]>
Date: Thu, 11 Nov 2021 10:27:06 +0000

> FP: nit s/value/values. Also I believe this could be misunderstood - can it be made more precise on the fact that only the values that are present in the received frame with the ACK flag set (and n…

This is not accurate: if the ACK flag is set then the payload of the
SETTINGS frame is required to be empty. There is no signal to send
whether a setting has been ignored. If it is necessary to get
confirmation that a setting has been understood, both parties will be
required to advertise it in their own SETTINGS frames.

---

From: Francesca Palombini <[email-masked]>
Date: Thu, 11 Nov 2021 10:56:34 +0000

Hi Cory! Thanks for the quick reaction! Merging your two emails.

Francesca

>> I have opened a github issue with the text below: https://github.com/httpwg/http2-spec/issues/974
>>
>> Francesca
>>
>>
>>
>> 1. -----
>>
>>
>>

FP: I see. My confusion is derived from the following sentence:

      The initial value of SETTINGS_ENABLE_PUSH is 1, which indicates
      that server push is permitted.

"server push is permitted" to me was not the same thing as "support receiving pushes", hence my misunderstanding that SETTINGS_ENABLE_PUSH for a server would mean that the server supports sending pushes, not receiving them.

>required to advertise it in their own SETTINGS frames.

FP: I see, so would you say "the sender of the settings can rely on the value been applied" is partly inaccurate (as settings might have been ignored as well)? Or maybe I don't know how to interpret "altered" here..

>not forbid the server from sending the frames, it just prevents them

FP: Ah, I missed that! Thanks for clarifying.

---

From: Cory Benfield <[email-masked]>
Date: Thu, 11 Nov 2021 11:05:07 +0000

> "server push is permitted" to me was not the same thing as "support receiving pushes", hence my misunderstanding that SETTINGS_ENABLE_PUSH for a server would mean that the server supports sending p…

Yeah, the initial value of 1 here is a bit tricky. That is historical,
from the first version of the document, and it's arguably in tension
with the original quote. The two together forbid servers from sending
the initial value in their SETTINGS frames, which is awkward.

It may be better for us to amend your newest quote to say something like:

    The initial value of SETTINGS_ENABLE_PUSH is 1. For clients this
    indicates that they are willing to receive PUSH_PROMISE frames. For
    servers this initial value has no effect.

Alternatively, we could say the initial value for servers is zero. We
did discuss this briefly:
https://github.com/httpwg/http2-spec/issues/793.

> FP: I see, so would you say "the sender of the settings can rely on the value been applied" is partly inaccurate (as settings might have been ignored as well)? Or maybe I don't know how to interpre…

Yeah I think we could add clarifying text that the value will be
applied if the peer recognises it, otherwise it will be ignored.

---

From: Francesca Palombini <[email-masked]>
Date: Fri, 12 Nov 2021 12:03:10 +0000

Hi Cory,

These clarifications would be great, IMO. By the way, I had a chat with Mark about the first two points, and as they won’t change the IETF Last Call text, I will move the document to last call right now.

Thanks,
Francesca

From: Cory Benfield <[email-masked]>
Date: Thursday, 11 November 2021 at 12:05
To: Francesca Palombini <[email-masked]>
Cc: [email-masked] <[email-masked]>, HTTP Working Group <[email-masked]>
Subject: Re: AD review of draft-ietf-httpbis-http2bis-05
On Thu, 11 Nov 2021 at 10:56, Francesca Palombini
<[email-masked]> wrote:
> >> I have opened a github issue with the text below: https://protect2.fireeye.com/v1/url?k=06b35d70-59286433-06b31deb-861fcb972bfc-66b3c761a3016880&q=1&e=de1449af-f134-4aa4-9ee8-15b033cdd19d&u=https%3A%2F%2Fgithub.com%2Fhttpwg%2Fhttp2-spec%2Fissues%2F974
>
> >>
>
> >> Francesca
>
> >>
>
> >>
>
> >>
>
> >> 1. -----
>
> >>
>
> >>
>
> >>
>

Alternatively, we could say the initial value for servers is zero. We
did discuss this briefly:
https://protect2.fireeye.com/v1/url?k=10cdb1e9-4f5688aa-10cdf172-861fcb972bfc-726d835c3917b4ab&q=1&e=de1449af-f134-4aa4-9ee8-15b033cdd19d&u=https%3A%2F%2Fgithub.com%2Fhttpwg%2Fhttp2-spec%2Fissues%2F793.

---

From: "Martin Thomson" <[email-masked]>
Date: Sat, 13 Nov 2021 00:58:05 +1100

Recall that this is a bis document.  We have a narrow scope for changes and so not everything here is new.

With that in mind, we should try to fix the issues you have identified.  I've opened a few pull requests.  I've trimmed your text where I think that it isn't necessary to discuss anything.  Most are straightforward, so while this was tedious, it's now done.  Thanks for reading through so carefully.

On Thu, Nov 11, 2021, at 08:23, Francesca Palombini wrote:
> 1. ----- https://github.com/httpwg/http2-spec/pull/977
> 2. ----- https://github.com/httpwg/http2-spec/pull/978
> 3. ----- https://github.com/httpwg/http2-spec/pull/979
> 4. ----- https://github.com/httpwg/http2-spec/pull/980
I chose to cite the section that defines frames rather than the IANA registry as this document only defines a subset of the frames in the registry.

> 5. ----- https://github.com/httpwg/http2-spec/pull/981
You are right to point out that our flow control text is a little soft.  I've tightened it up a little in this PR.

> 6. ----- https://github.com/httpwg/http2-spec/pull/982
As above, I've cited the section, not the registry.

> 7. ----- https://github.com/httpwg/http2-spec/pull/983
I've added a reference to the priority deprecation text to the first of these newly deprecated fields.

> 8. ----- Cory got this one.
> 9. ----- Cory got this one.
> 10. ----- no action necessary

A client that receives this from a server - and does not receive another SETTINGS in a reasonable time - cannot make new requests.  It might decide that it cannot continue to use the connection in that case.

> 11. ----- Cory got this one.
Note that ignoring a value (if allowed or required) is applying it.

> 12. ----- Cory got this one.
> 13. ----- Unless someone else picks this up, I'm going to pass on adding more references to the definition of error codes.
> 14. -----
> 

This is a term of art in HTTP and necessary knowledge.  I don't believe that a citation is necessary.

> 15. ----- https://github.com/httpwg/http2-spec/pull/984
Yeah, I think we fix simplify by removing this second, contradictory sentence.  Good catch.  (Edits made 5 years apart don't always guarantee consistency.)

> 16. ----- https://github.com/httpwg/http2-spec/pull/985
> 17. ----- Cory got this one.
> 18. ----- https://github.com/httpwg/http2-spec/pull/986
> 19. ----- Cory got this one.
> 20. -----

TCP is really terrible about this.  FIN is so fundamental a concept it just ends up spread all over.  Anything I might cite is not useful.  So not really.
 
> 21. ----- I don't plan to do anything about this.  These are pictures, not code. 
> 22. ----- Cory got this one.
> 23. ----- Cory got this one.
> 24. ----- https://github.com/httpwg/http2-spec/pull/987
> 25. ---- https://github.com/httpwg/http2-spec/pull/988

---

From: Francesca Palombini <[email-masked]>
Date: Sat, 13 Nov 2021 10:49:47 +0000

Hi Martin! Thanks to you and Cory for considering my comments, I know these were mostly straightforward, and sorry it was tedious, I do hope they bring some additional value, however small, to the document.

I went through the PRs and I answered to some of the points below inline. The rest, as you say, did not need more discussion. I also checked off in https://github.com/httpwg/http2-spec/issues/974 the points which I think are dealt with.

As you can see I left 1. and 7. unchecked just so I remember which comments I have further questions on.

Thanks again, and feel free to merge all of the PRs (except 977 - let's confirm this is what we want first).

Francesca

From: Martin Thomson <[email-masked]>
Date: Friday, 12 November 2021 at 15:02
To: [email-masked] <[email-masked]>
Subject: Re: AD review of draft-ietf-httpbis-http2bis-05
>> 1. ----- https://github.com/httpwg/http2-spec/pull/977

FP: Thank you. However I just wanted to note here that, although my preference in general would be to take the most updated version of a specification, the TCPbis doc currently sits with a big number of DISCUSS comments since end of September. I think the wg should consider this when deciding which reference to go with. I'd like it to be a wg decision since I am pretty sure the question "should this document reference TCPbis?" will come up in IESG evaluation (hence why I brought it up) and I'd like us to be prepared for it. If the wg agrees to go forward with referencing TCPbis I can coordinate with Martin Duke and authors to see what's the expected timeline.

>I chose to cite the section that defines frames rather than the IANA registry as this document only defines a subset of the frames in the registry.

FP: That's fine, thank you.

>You are right to point out that our flow control text is a little soft.  I've tightened it up a little in this PR.

FP: Thank you! Looks good.

>I've added a reference to the priority deprecation text to the first of these newly deprecated fields.

FP: Great, thank you. I had some questions about what part of the handling for priorities is still mandatory to support, following my reading of draft-ietf-httpbis-priority (see point 2 of https://mailarchive.ietf.org/arch/msg/httpbisa/mLM0RujKL6ZXY4eQtbQo2Xomf7c/ ). Maybe you could help clarifying it for me? I read this section as well as 5.3, and although the fields are defined, since they are deprecated there is no processing defined, except for errors. What am I missing?

>A client that receives this from a server - and does not receive another SETTINGS in a reasonable time - cannot make new requests.  It might decide that it cannot continue to use the connection in t…

FP: That makes sense, thanks for clarifying.

>Note that ignoring a value (if allowed or required) is applying it.

FP: Ah, that could be interpreted like that.. it is tricky though to say the client can rely on the values having been applied, and that could mean all its settings have been ignored. Not insisting, just clarifying my interpretation :) If no one else sees this as misinterpretable, I am fine as is (and thanks Cory for polishing the text).

>> 13. ----- Unless someone else picks this up, I'm going to pass on adding more references to the definition of error codes.

FP: Ok.

>This is a term of art in HTTP and necessary knowledge.  I don't believe that a citation is necessary.

FP: What I was looking for was a reference to Section 15.2 of [HTTP], but I am fine either way.

>Yeah, I think we fix simplify by removing this second, contradictory sentence.  Good catch.  (Edits made 5 years apart don't always guarantee consistency.)

FP: Glad this comment was useful! :)

>TCP is really terrible about this.  FIN is so fundamental a concept it just ends up spread all over.  Anything I might cite is not useful.  So not really.

FP: Ok, that's fine. Just checking :)

>> 21. ----- I don't plan to do anything about this.  These are pictures, not code.

FP: Ok, sure. But we all know how important examples are.

---

From: "Martin Thomson" <[email-masked]>
Date: Mon, 15 Nov 2021 15:10:17 +1100

On Sat, Nov 13, 2021, at 21:49, Francesca Palombini wrote:
> Hi Martin! Thanks to you and Cory for considering my comments, I know 
> these were mostly straightforward, and sorry it was tedious, I do hope 
> they bring some additional value, however small, to the document.

Only tedious because that's the nature of the job.  I do appreciate how clear and actionable your input has been.

> [...] the TCPbis doc currently sits with a big number of 
> DISCUSS comments since end of September. 

My opinion: we should drop the dependency unless TCPbis suddenly resolves things.  We can keep the pull request open until that happens.  We don't depend on any of the details of TCP, so it's OK to have an outdated reference.
 
> FP: Great, thank you. I had some questions about what part of the 
> handling for priorities is still mandatory to support, following my 
> reading of draft-ietf-httpbis-priority (see point 2 of 
> https://mailarchive.ietf.org/arch/msg/httpbisa/mLM0RujKL6ZXY4eQtbQo2Xomf7c/ 
> ). Maybe you could help clarifying it for me? I read this section as 
> well as 5.3, and although the fields are defined, since they are 
> deprecated there is no processing defined, except for errors. What am I 
> missing?

I think that's a problem in the -priority draft more than this one, though maybe Section 5.3 could be a tiny bit clearer.

What this is supposed to say, and what I think that it does say, is:

~~~
 HTTP/2 signals are pretty bad, but no signals is almost certainly worse. So,

1. Use newer, better signals if you can.
2. Use HTTP/2 signals if you don't have anything better.
3. Use ANYTHING else in place of RFC 7540 default priorities.

"Use" being both send and receive.
~~~

I think that the priority draft wants to say the same thing, but you caught some unfortunate word choices that might make that less clear.  No doubt there are similar choices in this document that I'm just not seeing.

---

From: Francesca Palombini <[email-masked]>
Date: Thu, 18 Nov 2021 15:47:49 +0000

Hi Martin,

Thank you for all the updates, you can go ahead and merge them as soon as it’s convenient and submit the new version, so that reviewers from Last Call can read the latest version.

Re- dependencies to TCPbis – I am ok with not having it. Now that we have had the discussion, the wg is aware and I have not heard any voice in favor of updating the reference, so please keep it as is.

Re- priority signals: thank you, the text below and re-reading 5.3.2 helped me understand the hierarchy of possible choices. Mike also answered some of my confusion in https://github.com/httpwg/http-extensions/issues/1783#issuecomment-971693935 – up to you if you think it is useful to add some of that regarding what parts are still mandatory to implement, I’m fine either way.

Thanks again!
Francesca

From: Martin Thomson <[email-masked]>
Date: Monday, 15 November 2021 at 05:14
To: [email-masked] <[email-masked]>
Subject: Re: AD review of draft-ietf-httpbis-http2bis-05

>>> 7. ----- https://protect2.fireeye.com/v1/url?k=f7306dd5-a8ab54d0-f7302d4e-86d2114eab2f-88675e722e367ee7&q=1&e=00a5acd9-7b14-4397-8e3f-7688ec22c3fb&u=https%3A%2F%2Fgithub.com%2Fhttpwg%2Fhttp2-spec%2Fpull%2F983

~~~