Copilot commented on code in PR #1394:
URL: https://github.com/apache/pekko-http/pull/1394#discussion_r4483882219
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
+
+ /**
+ * @since 2.0.0
+ */
+ def withFrameTypeThrottleFrameTypes(frameTypes: java.util.Set[String]):
Http2ClientSettings =
+ copy(frameTypeThrottleFrameTypes = frameTypes.asScala.toSet)
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
+
+ /**
+ * @since 2.0.0
+ */
+ def withFrameTypeThrottleFrameTypes(frameTypes: java.util.Set[String]):
Http2ClientSettings =
+ copy(frameTypeThrottleFrameTypes = frameTypes.asScala.toSet)
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleCost(): Int = frameTypeThrottleCost
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleBurst(): Int = frameTypeThrottleBurst
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleInterval: Duration =
Duration.ofMillis(frameTypeThrottleInterval.toMillis)
+
+ /**
+ * @since 2.0.0
+ */
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
+
+ /**
+ * @since 2.0.0
+ */
+ def withFrameTypeThrottleFrameTypes(frameTypes: java.util.Set[String]):
Http2ClientSettings =
+ copy(frameTypeThrottleFrameTypes = frameTypes.asScala.toSet)
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleCost(): Int = frameTypeThrottleCost
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
+
+ /**
+ * @since 2.0.0
+ */
+ def withFrameTypeThrottleFrameTypes(frameTypes: java.util.Set[String]):
Http2ClientSettings =
+ copy(frameTypeThrottleFrameTypes = frameTypes.asScala.toSet)
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleCost(): Int = frameTypeThrottleCost
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleBurst(): Int = frameTypeThrottleBurst
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleInterval: Duration =
Duration.ofMillis(frameTypeThrottleInterval.toMillis)
+
+ /**
+ * @since 2.0.0
+ */
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http-core/src/main/scala/org/apache/pekko/http/javadsl/settings/Http2ClientSettings.scala:
##########
@@ -126,4 +127,36 @@ trait Http2ClientSettings { self:
scaladsl.settings.Http2ClientSettings.Http2Cli
def withMaxConnectionBackoff(backoff: Duration): Http2ClientSettings =
copy(maxConnectionBackoff = backoff.toMillis.millis)
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleFrameTypes(): java.util.Set[String] =
frameTypeThrottleFrameTypes.asJava
+
+ /**
+ * @since 2.0.0
+ */
+ def withFrameTypeThrottleFrameTypes(frameTypes: java.util.Set[String]):
Http2ClientSettings =
+ copy(frameTypeThrottleFrameTypes = frameTypes.asScala.toSet)
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleCost(): Int = frameTypeThrottleCost
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleBurst(): Int = frameTypeThrottleBurst
+
+ /**
+ * @since 2.0.0
+ */
+ def getFrameTypeThrottleInterval: Duration =
Duration.ofMillis(frameTypeThrottleInterval.toMillis)
+
+ /**
+ * @since 2.0.0
+ */
Review Comment:
The Java DSL exposes getters for `cost`/`burst` but does not expose
corresponding `withFrameTypeThrottleCost(...)` /
`withFrameTypeThrottleBurst(...)` setters, which makes the new throttle
partially non-configurable from Java. Consider adding the missing `with...`
methods for parity with the Scala DSL. Also, `getFrameTypeThrottleInterval` is
the only getter declared without `()`, which is inconsistent with the other
Java-style getters in this trait—prefer `def getFrameTypeThrottleInterval():
Duration = ...` for consistency.
##########
http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientSpec.scala:
##########
@@ -836,6 +837,52 @@ class Http2ClientSpec extends PekkoSpecWithMaterializer("""
})
}
+ "throttle frame types" should {
+ val throttleExceeded = "HTTP2 connection failed with error [Maximum
throttle throughput exceeded.]"
+
+ abstract class WaitingForResponseData extends TestSetup with NetProbes {
+ user.emitRequest(Get("/"))
+ val streamId = network.expect[HeadersFrame]().streamId
+ network.sendHEADERS(streamId, endStream = false,
Seq(RawHeader(":status", "200")))
+ val entityDataIn =
ByteStringSinkProbe(user.expectResponse().entity.dataBytes)
+
+ def emptyDataFrames(count: Int): ByteString =
+ Seq.fill(count)(FrameRenderer.renderFrame(FrameType.DATA,
ByteFlag.Zero, streamId, ByteString.empty))
+ .reduce(_ ++ _)
Review Comment:
Building large `ByteString`s using `Seq.fill(...).reduce(_ ++ _)` repeatedly
concatenates and can become O(n^2) due to copying. It also throws if called
with `count == 0`. Consider using a `ByteString.Builder` (or equivalent) to
append chunks efficiently and safely.
##########
http-core/src/main/scala/org/apache/pekko/http/scaladsl/settings/Http2ServerSettings.scala:
##########
@@ -435,6 +484,7 @@ object Http2ClientSettings extends
SettingsCompanion[Http2ClientSettings] {
"persistent-connection-max-age-jitter must be >= 0 and < 1")
require(completionTimeout > Duration.Zero, "completion-timeout must be >
0")
require(baseConnectionBackoff <= maxConnectionBackoff,
"base-connection-backoff must be <= max-connection-backoff")
Review Comment:
Only `frame-type-throttle.interval` is validated here. Akka/Pekko Streams
`throttle` requires a positive `cost` and a non-negative `burst`; if a user
misconfigures `cost <= 0` (or `burst < 0`), the stream will fail at
materialization/runtime with an exception. Recommend adding explicit
`require(frameTypeThrottleCost > 0, ...)` and `require(frameTypeThrottleBurst
>= 0, ...)` (or `> 0`, depending on intended semantics) to fail fast with a
clear configuration error.
##########
http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ClientSpec.scala:
##########
@@ -836,6 +837,52 @@ class Http2ClientSpec extends PekkoSpecWithMaterializer("""
})
}
+ "throttle frame types" should {
+ val throttleExceeded = "HTTP2 connection failed with error [Maximum
throttle throughput exceeded.]"
+
+ abstract class WaitingForResponseData extends TestSetup with NetProbes {
+ user.emitRequest(Get("/"))
+ val streamId = network.expect[HeadersFrame]().streamId
+ network.sendHEADERS(streamId, endStream = false,
Seq(RawHeader(":status", "200")))
+ val entityDataIn =
ByteStringSinkProbe(user.expectResponse().entity.dataBytes)
+
+ def emptyDataFrames(count: Int): ByteString =
+ Seq.fill(count)(FrameRenderer.renderFrame(FrameType.DATA,
ByteFlag.Zero, streamId, ByteString.empty))
+ .reduce(_ ++ _)
+ }
+
+ "fail the connection when flooded with RST_STREAM frames by
default".inAssertAllStagesStopped(
+ new TestSetup with NetProbes {
+ user.emitRequest(Get("/"))
+ val streamId = network.expect[HeadersFrame]().streamId
+ val rstFrame = FrameRenderer.render(RstStreamFrame(streamId,
ErrorCode.CANCEL))
+
+ EventFilter.error(start = throttleExceeded, occurrences =
1).intercept {
+ network.sendBytes(Seq.fill(1000)(rstFrame).reduce(_ ++ _))
Review Comment:
Building large `ByteString`s using `Seq.fill(...).reduce(_ ++ _)` repeatedly
concatenates and can become O(n^2) due to copying. It also throws if called
with `count == 0`. Consider using a `ByteString.Builder` (or equivalent) to
append chunks efficiently and safely.
##########
http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerEmptyDataThrottleSpec.scala:
##########
@@ -35,10 +37,17 @@ class Http2ServerEmptyDataThrottleSpec extends
Http2SpecWithMaterializer("""
"HTTP2 connection failed with error [Maximum throttle throughput
exceeded.]. Sending INTERNAL_ERROR and closing connection.")
"The Http/2 server implementation" should {
- "cancel connection when flooded with empty DATA
frames".inAssertAllStagesStopped(
+ "cancel connection when flooded with empty DATA frames that end their
stream".inAssertAllStagesStopped(
new TestSetup with RequestResponseProbes {
- val emptyDataFrame = FrameRenderer.renderFrame(FrameType.DATA,
ByteFlag.Zero, 1, ByteString.empty)
- network.sendBytes(Seq.fill(1000)(emptyDataFrame).reduce(_ ++ _))
+ // each request is opened with HEADERS and closed with an empty DATA
frame carrying END_STREAM, which only
+ // "empty-data" charges; 200 streams stays below
max-concurrent-streams but exceeds the burst of 100
+ val requests = (0 until 200).map { i =>
+ val streamId = 2 * i + 1
+ val headerBlock =
network.encodeRequestHeaders(HttpRequest(HttpMethods.POST))
+ FrameRenderer.render(HeadersFrame(streamId, endStream = false,
endHeaders = true, headerBlock, None)) ++
+ FrameRenderer.renderFrame(FrameType.DATA,
Http2Protocol.Flags.END_STREAM, streamId, ByteString.empty)
+ }
+ network.sendBytes(requests.reduce(_ ++ _))
Review Comment:
As with other specs, `requests.reduce(_ ++ _)` can be quadratic due to
repeated `ByteString` concatenation. Since this is preparing a fairly large
payload, consider assembling bytes via a `ByteString.Builder` (or similar) to
keep test runtime stable and avoid unnecessary allocations.
##########
http2-tests/src/test/scala/org/apache/pekko/http/impl/engine/http2/Http2ServerEmptyDataNoEndStreamThrottleSpec.scala:
##########
@@ -38,6 +39,8 @@ class Http2ServerEmptyDataNoEndStreamThrottleSpec extends
Http2SpecWithMateriali
"The Http/2 server implementation" should {
"cancel connection when flooded with empty DATA frames that do not end the
stream".inAssertAllStagesStopped(
new TestSetup with RequestResponseProbes {
+ // open the stream first, so the flood arrives on an open stream
rather than an idle one
+ network.sendRequestHEADERS(1, HttpRequest(HttpMethods.POST), endStream
= false)
val emptyDataFrame = FrameRenderer.renderFrame(FrameType.DATA,
ByteFlag.Zero, 1, ByteString.empty)
network.sendBytes(Seq.fill(1000)(emptyDataFrame).reduce(_ ++ _))
Review Comment:
`Seq.fill(1000)(...).reduce(_ ++ _)` can be expensive due to repeated
copying during concatenation. Prefer a `ByteString.Builder` (or similar) to
assemble the outbound bytes in linear time.
##########
http-core/src/main/scala/org/apache/pekko/http/impl/engine/http2/Http2Blueprint.scala:
##########
@@ -141,10 +141,12 @@ private[http] object Http2Blueprint {
FrameLogger.logFramesIfEnabled(settings.http2Settings.logFrames) atop //
enable for debugging
hpackCoding(masterHttpHeaderParser, settings.parserSettings,
settings.http2Settings.maxHeaderListSize)
- val frameTypesForThrottle =
getFrameTypesForThrottle(settings.http2Settings)
+ val http2Settings = settings.http2Settings
+ val frameTypesForThrottle =
getFrameTypesForThrottle(http2Settings.frameTypeThrottleFrameTypes)
val flowWithPossibleThrottle = if (frameTypesForThrottle.nonEmpty) {
- (initialFlow atop rapidResetMitigation(settings.http2Settings,
frameTypesForThrottle))
+ (initialFlow atop frameTypeThrottle(frameTypesForThrottle,
http2Settings.frameTypeThrottleCost,
+ http2Settings.frameTypeThrottleInterval,
http2Settings.frameTypeThrottleBurst))
.atopKeepLeft(framing(log, settings.http2Settings.maxFrameSize))
} else initialFlow atop framing(log, settings.http2Settings.maxFrameSize)
Review Comment:
This block introduces `http2Settings` but still references
`settings.http2Settings.maxFrameSize` on lines 150–151. Using
`http2Settings.maxFrameSize` consistently would reduce repetition and make
future refactors less error-prone.
--
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]
