pjfanning opened a new pull request, #1239: URL: https://github.com/apache/pekko-http/pull/1239
### Motivation Builds on #1238. That PR added HTTP/2 HEAD coverage and pinned three defects with `FIXME`s; this one fixes them and flips those assertions. **It contains #1238's commit**, because the tests-only branch lives on a fork and cannot be used as a base here — review the second commit for the fix, or merge #1238 first and this rebases to just that commit. The three defects: 1. **The response entity was emitted as DATA frames.** RFC 9110 §9.3.2: *"the server MUST NOT send content in a response to a HEAD request."* 2. **`transparent-head-requests` was ignored** over HTTP/2 — it is applied only by `HttpServerBluePrint`, the HTTP/1.1 blueprint. 3. **A `304` (and a `204`) got `content-length: 0`** where HTTP/1.1 omits the header. RFC 9110 §15.4.5 expects a `304` to carry the `Content-Length` a `200` would have had, so a made-up zero is actively misleading. The hard part is (1): `ResponseRendering` only ever sees an `HttpResponse` plus its stream-id attribute, so it cannot know the request method. Carrying it on an attribute alongside `Http2.streamId` would only fix the `bind` API — `bindFlow` users copy that attribute by hand, and `Http2ServerSpec`'s own probes show what such a user looks like. ### Modification **Track the method where it is already known per connection**, below the HTTP layer: - `Http2StreamHandling` records the stream ids of incoming HEAD requests as the request HEADERS frame opens the stream (`:method` is already parsed to an `HttpMethod` by `HeaderDecompression`, and `Http2StreamHandling` is per-connection state, so no new plumbing is needed). - `handleOutgoingCreated` cancels the response data for those streams and sends the initial headers with `endStream` set, via a new `Http2SubStream.withoutData`. **The header pairs are left untouched**, so the peer still learns the `content-length` it would have received for a GET — the same split HTTP/1.1 makes between rendering the length and discarding the bytes. - Entries are removed when the response is created and when the stream closes (both the `Closed` transition in `updateStateAndReturn` and the `onDownstreamFinish` path), so the set cannot outlive its streams. Because that tracking reads the method off the wire *below* the HTTP layer, it is unaffected by anything the HTTP layer does to the request — which makes (2) fall out cheaply: `RequestParsing` now rewrites HEAD to GET when `transparent-head-requests` is on, exactly as `HttpServerBluePrint.scala:159` does for HTTP/1.1, and the body is still stripped because the demux remembers the wire method. For (3), `ResponseRendering` gains the status-based part of the rules HTTP/1.1 applies through `HttpMethod.contentLengthAllowed`, so 1xx, 204 and 304 no longer render a `content-length`. Works for `bindFlow` as well as `bind`, since nothing depends on the handler propagating an attribute. ### Result A HEAD request over HTTP/2 gets headers only, carrying the `content-length` the resource would have had, and `transparent-head-requests` behaves as it does for HTTP/1.1. Two deliberate scope limits, both worth a follow-up rather than widening this PR: - **h2c upgrade.** In the `Http2.scala:191` upgrade path the request is parsed by the HTTP/1.1 parser and injected on stream 1, so it never passes a HEADERS frame through the demux and a HEAD there would still get a body. HEAD combined with an h2c upgrade is very unusual. - **Empty entities.** A HEAD response with an empty entity still renders `content-length: 0` over HTTP/2, where HTTP/1.1 omits it (see #1237). Aligning that needs the request method at rendering time, which is the plumbing this PR deliberately avoids. ### Tests The three `FIXME` tests from #1238 flip to the correct expectations, and three more are added — a streamed response entity (asserting the source is cancelled and no DATA is sent), transparent-head-requests translating to GET while still stripping the body, and a 204. - `sbt "http2-tests / test"` — 349 passed, 25 ignored, 26 pending (ignored/pending are pre-existing) - `sbt +mimaReportBinaryIssues` — success - `scalafmt --mode diff-ref=upstream/main` — clean; `git diff --check` — clean All changed files are `private[http2]` internals, so there is no API surface change. ### References Refs #1236, Refs #1238 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
