Re: Benjamin Kaduk's Discuss on draft-ietf-httpbis-http2bis-06: (with DISCUSS and COMMENT)

On Fri, Jan 07, 2022 at 03:13:46PM +1100, Martin Thomson wrote:
> For your comments, I have a few responses inline.  I've deleted those that I more or less directly accepted.  There were some good ones there, but I see no need to add to the size of this email.  See the pull request for details:

Understandable, and thanks.

I will also trim the bits about HTTP/2 vs HTTP/3; the common them of
"HTTP/3 is allowed to (and expected to!) be better than HTTP/2" is
well-taken, and "no change" seems reasonable as indicated.  Hopefully we
can hear from Mike where his input was requested.

> https://github.com/httpwg/http2-spec/pull/1012
> 
> On Thu, Jan 6, 2022, at 04:43, Benjamin Kaduk via Datatracker wrote:
> >    *  The ranges of codepoints for settings and frame types that were
> >       reserved for "Experimental Use" are now available for general use.
> 
> Good catch.  That was lost in a refactoring of the IANA Considerations section.  That was in error.
> 
> https://github.com/httpwg/http2-spec/pull/1011

Looks like an easy fix; excellent.

> > Section 5.1
> >
> >    half-closed (local):  A stream that is in the "half-closed (local)"
> >       [...]
> >       An endpoint can receive any type of frame in this state.
> >       [...]
> >       PRIORITY frames can be received in this state.
> >
> > I'm not sure whether this redundancy is adding much.  In RFC 7540 the last
> > sentence was followed by a note about "used to reprioritize streams that
> > depend on the identified stream", but since PRIORITY is basically neutered
> > now it may not need a special mention.
> 
> I would prefer to keep this as it is a common error.  It's easy to forget to allow this, especially now that PRIORITY is deprecated.

Ok, that's a fine reason to have a special mention.

> >    closed:  The "closed" state is the terminal state.
> >    [...]
> >       An endpoint that sends a frame with the END_STREAM flag set or a
> >       RST_STREAM frame might receive a WINDOW_UPDATE or RST_STREAM frame
> >       from its peer in the time before the peer receives and processes
> >       the frame that closes the stream.
> >
> > Could such a client also receive PUSH_PROMISE frames in this situation?
> > The following paragraph (about the "open"->"closed" via sending RST_STREAM
> > transition) does mention receiving any frame (and PUSH_PROMISE specifically)
> > and the need to update compression state, but if I understand correctly
> > PUSH_PROMISE can happen after a "half-closed (local)"->"closed" transition
> > due to sending RST_STREAM as well, which does not seem covered by the
> > existing text.
> 
> The next paragraph covers that case, but neglected to mention "half-closed (local)" in that, which it should have.  I think that covers your other points about this.

I suspect you are right about that; the "half-closed (local)" state is very
similar to the "open" state in terms of what we would expect to receive.

The only part of my comment that this might not resolve is:

% in 7540 there was a dedicated paragraph about PUSH_PROMISE that went on
% to note that RST_STREAM would be needed to close an unwanted promised
% stream; I don't know if that is worth bringing back or not.

I would be willing to believe that we talk about this topic elsewhere and
it doesn't need to be covered here, though.

> > Section 5.1.2
> >
> >    An endpoint that wishes to reduce the value of
> >    SETTINGS_MAX_CONCURRENT_STREAMS to a value that is below the current
> >    number of open streams can either close streams that exceed the new
> >    value or allow streams to complete.
> >
> > Is there a race condition here between the peer continuously creating new
> > connections and the endpoint closing connections to try to lower the value?
> > In particular, what happens if the SETTINGS that reduces the value crosses
> > on the wire with the frame creating a stream that would exceed the value?
> 
> The new limit only applies after the peer acknowledges the change, so as long as they are within the limit that is currently acknowledged, they are not in violation of the protocol.  This is really talking about what to do about those that are nominally within the limit, but in excess of what the endpoint really wants.

Okay, thanks for the explanation (and reminder that the acknowledgment is
needed before it takes effect).

> > Section 8.4.1
> >
> >    The header fields in PUSH_PROMISE and any subsequent CONTINUATION
> >    frames MUST be a valid and complete set of request header fields
> >    (Section 8.3.1).  The server MUST include a method in the :method
> >    pseudo-header field that is safe and cacheable.  If a client receives
> >    a PUSH_PROMISE that does not include a complete and valid set of
> >    header fields or the :method pseudo-header field identifies a method
> >    that is not safe, it MUST respond with a stream error (Section 5.4.2)
> >    of type PROTOCOL_ERROR.
> >
> > Up in §8.4 we seemed to talk about the same (non-safe) scenario by saying
> > that the promised stream being reset with the PROTOCOL_ERROR.  But I read
> > this text ("respond with a stream error") as applying to the explicit
> > request to which the promised request is associated.  Is that the intent?
> > If not, perhaps we should say "respond on the promised stream ID".
> 
> Good question.  The error exists with the promise, not the request, so I think the latter.

That was my thought as well, but I wasn't confident enough to just put it
in my PR and claim it to be "editorial" :)

Thanks again,

Ben

Received on Saturday, 8 January 2022 02:36:12 UTC