On Wed, 18 Jan 2023 11:47:06 GMT, Conor Cleary <[email protected]> wrote:
> The changes in this PR look good to me. While looking at the source code, I > noticed that the `Stream` class has a `PushedStream` nested class which too > has a similar implementation in `handleResponse()`. I haven't yet checked the > spec related to this; do you know if this code too will need a similar fix? The original situation for a regular request-response in http2 was caused by a server responding to a client request with response headers that included only a code 200 status with no flags set and the related data. This was then followed by trailing headers with both the END_STREAM and END_HEADERS flags set with an empty header block (though the contents of the block aren't of concern here). Now, the Push Promise version of this situation is a bit different. A Push Promise frame is sent to the client containing all expected details such as the promisedStreamId, request path etc. However if we try mirror the previous situation by _not_ including an END_HEADERS flag in the Push Promise frame and attempting to send a trailing HEADERS frame, the HttpClient correcly throws a protocol error specifying that the client `Expected a Continuation frame but received HEADERS`. This is behavior defined in [section 6.6. of RFC7540: HTTP/2](https://www.rfc-editor.org/rfc/rfc7540#section-6.6). The PUSH_PROMISE frame defines the following flags: END_HEADERS (0x4): When set, bit 2 indicates that this frame contains an entire header block (Section 4.3) and is not followed by any CONTINUATION frames. A PUSH_PROMISE frame without the END_HEADERS flag set MUST be followed by a CONTINUATION frame for the same stream. A receiver MUST treat the receipt of any other type of frame or a frame on a different stream as a connection error (Section 5.4.1) of type PROTOCOL_ERROR. With this in mind, I would argue that in the case of `PushedStream::handleResponse` no changes are required as the behavior is quite well defined and strict when it comes to testing what the HttpClient will allow. One interesting side note is that END_STREAM is not strictly speaking a defined flag for PUSH_PROMISE frames. Interested to see what reviewers have to say about that analysis or if there are any mistakes in my thinking, it was quite a tricky one to wrap my head around given the differences with how Push Requests are treated by the client. ------------- PR: https://git.openjdk.org/jdk/pull/12028
