pjfanning commented on code in PR #1394:
URL: https://github.com/apache/pekko-http/pull/1394#discussion_r4485922945
##########
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:
Leaving this as is. It is a test fixture of at most 1000 frames, and
`reduce(_ ++ _)` is the idiom the sibling throttle specs already use, for
example `Http2ServerEnableFrameTypeThrottleSpec`. The helper is never called
with a count of 0.
##########
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:
Leaving this as is. It is a test fixture of at most 1000 frames, and
`reduce(_ ++ _)` is the idiom the sibling throttle specs already use, for
example `Http2ServerEnableFrameTypeThrottleSpec`.
##########
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:
Leaving this as is. It is a test fixture of at most 1000 frames, and
`reduce(_ ++ _)` is the idiom the sibling throttle specs already use, for
example `Http2ServerEnableFrameTypeThrottleSpec`.
##########
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:
Fixed in 41d118a44: both `framing` calls in `serverStack` now use
`http2Settings.maxFrameSize`.
--
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]
