Skip to content

avoid ByteString when parsing HTTP/2 headers #800

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
Draft

avoid ByteString when parsing HTTP/2 headers #800

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f85850
avoid ByteString when parsing HTTP/2 headers
pjfanning Sep 26, 2025
12a3c94
scalafmt
pjfanning Sep 26, 2025
a1ac952
workaround
pjfanning Oct 2, 2025
7e3c229
Clarify pseudo-header handling in RequestParsing
pjfanning Oct 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Demux.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,12 @@ private[http2] class Http2ClientDemux(http2Settings: Http2ClientSettings, master
extends Http2Demux(http2Settings, initialRemoteSettings = Nil, upgraded = false, isServer = false) {

def wrapTrailingHeaders(headers: ParsedHeadersFrame): Option[ChunkStreamPart] = {
val headerParser = masterHttpHeaderParser.createShallowCopy()
Some(LastChunk(extension = "",
headers.keyValuePairs.map {
case (name, value: HttpHeader) => value
case (name, value) => parseHeaderPair(headerParser, name, value.asInstanceOf[String])
case (name, value) =>
val headerParser = masterHttpHeaderParser.createShallowCopy()
parseHeaderPair(headerParser, name, value.asInstanceOf[String])
}.toList))
}

Expand Down
23 changes: 15 additions & 8 deletions http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/RequestParsing.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import pekko.http.scaladsl.model._
import pekko.http.scaladsl.model.headers.`Tls-Session-Info`
import pekko.http.scaladsl.settings.ServerSettings
import pekko.stream.Attributes
import pekko.util.ByteString
import pekko.util.OptionVal

import scala.annotation.tailrec
Expand Down Expand Up @@ -191,13 +190,21 @@ private[http2] object RequestParsing {
}

private[http2] def parseHeaderPair(httpHeaderParser: HttpHeaderParser, name: String, value: String): HttpHeader = {
// FIXME: later modify by adding HttpHeaderParser.parseHttp2Header that would use (name, value) pair directly
// or use a separate, simpler, parser for Http2
// The odd-looking 'x' below is a by-product of how current parser and HTTP/1.1 work.
// Without '\r\n\x' (x being any additional byte) parsing will fail. See HttpHeaderParserSpec for examples.
val concHeaderLine = name + ": " + value + "\r\nx"
httpHeaderParser.parseHeaderLine(ByteString(concHeaderLine))()
httpHeaderParser.resultHeader
import HttpHeader.ParsingResult
if (name.startsWith(":")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe just a look ahead char is better

// HttpHeader.parse used in `else` block does not support pseudo-headers (that have ':' prefix in header name)
// The odd-looking 'x' below is a by-product of how current parser and HTTP/1.1 work.
// Without '\r\n\x' (x being any additional byte) parsing will fail. See HttpHeaderParserSpec for examples.
val concHeaderLine = s"$name: $value\r\nx"
httpHeaderParser.parseHeaderLine(pekko.util.ByteString(concHeaderLine))()
httpHeaderParser.resultHeader
} else {
HttpHeader.parse(name, value, httpHeaderParser.settings) match {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general that's really cleaner. I don't expect a performance hit, there's a bit of a difference of how the parsers are looked up between HttpHeader.parse and httpHeaderParser.parseHeaderLine. HttpHeader.parse uses binary search to find the right parser and httpHeaderParser.parseHeaderLine uses kind of a lut / radix lookup once the parser is primed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @jrudolph.
I had to use the startsWith(":") check because HttpHeader.parse doesn't support pseudo headers and 1 test includes a :grpc-status pseudo header.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's annoying

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wait, does it even parse it into anything useful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll debug it later. There is an existing test that relies on the header parser not failing. Whether it needs the parser to actually process the pseudo header at all is something that I haven't yet tested.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's a typo in the test? Is :grpc-status a thing, i.e. is it allowed for gRPC (outside the HTTP/2 spec) to extend the set of : pseudo headers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pekko-grpc also doesn't use the colon form so maybe it's just a typo and we could just fix the test.

case ParsingResult.Ok(header, errors) if errors.isEmpty => header
case ParsingResult.Ok(_, errors) => throw ParsingException(errors.head)
case ParsingResult.Error(info) => throw ParsingException(info)
}
}
}

private[http2] def checkRequiredPseudoHeader(name: String, value: AnyRef): Unit =
Expand Down
Loading