Enable OgHttp2Session to RST_STREAM overflow streams with unacknowledged MAX_CONCURRENT_STREAMS values.
This CL adds a pending max inbound concurrent streams field to OgHttp2Session,
which stores the pending MAX_CONCURRENT_STREAMS value sent by OgHttp2Session
but not yet acknowledged by the peer. Then OgHttp2Session can reject incoming
streams exceeding the value with a RST_STREAM REFUSED_STREAM (not GOAWAY),
which better aligns oghttp2 with nghttp2 behavior.
This change affects at least two codec_impl_test tests:
http://sponge2/d291d8b8-2281-48b2-ae8f-203c94b0f484
If test mutual dispatch resolution is merged
(https://github.com/envoyproxy/envoy/pull/19195),
Http2CodecImplStreamLimitTest.LazyDecreaseMaxConcurrentStreamsConsumeError
passes, and
Http2CodecImplStreamLimitTest.LazyDecreaseMaxConcurrentStreamsIgnoreError
passes mod some nghttp2-specific error message [*].
[*] http://google3/third_party/envoy/src/source/common/http/http2/codec_impl.cc;l=850;rcl=410881808
PiperOrigin-RevId: 414519202
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc
index fae5d11..23322cb 100644
--- a/http2/adapter/nghttp2_adapter_test.cc
+++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -3243,7 +3243,7 @@
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
EXPECT_EQ(stream_frames.size(), stream_result);
- // Apparently nghttp2 sends a GOAWAY for this error, even though
+ // The server should send a GOAWAY for this error, even though
// OnInvalidFrame() returns true.
EXPECT_TRUE(adapter->want_write());
EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
@@ -3289,8 +3289,8 @@
visitor.Clear();
// Let the client avoid sending a SETTINGS ack and attempt to open more than
- // the advertised number of streams. Apparently nghttp2 still rejects the
- // overflow stream, albeit with a RST_STREAM by default instead of a GOAWAY.
+ // the advertised number of streams. The server should still reject the
+ // overflow stream, albeit with RST_STREAM REFUSED_STREAM instead of GOAWAY.
const std::string stream_frames =
TestFrameSequence()
.Headers(1,
@@ -3318,7 +3318,7 @@
3, Http2VisitorInterface::InvalidFrameError::kRefusedStream));
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
- EXPECT_EQ(stream_frames.size(), stream_result);
+ EXPECT_EQ(stream_result, stream_frames.size());
// The server sends a RST_STREAM for the offending stream.
EXPECT_TRUE(adapter->want_write());
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index b8e4c61..ea6eaf3 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -3083,23 +3083,30 @@
EXPECT_CALL(
visitor,
OnInvalidFrame(3, Http2VisitorInterface::InvalidFrameError::kProtocol));
+ // The oghttp2 stack also signals the connection error via OnConnectionError()
+ // and a negative ProcessBytes() return value.
+ EXPECT_CALL(visitor,
+ OnConnectionError(Http2VisitorInterface::ConnectionError::
+ kExceededMaxConcurrentStreams));
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
- EXPECT_EQ(static_cast<size_t>(stream_result), stream_frames.size());
+ EXPECT_LT(stream_result, 0);
- // The server should send a RST_STREAM for the offending stream.
+ // The server should send a GOAWAY for this error, even though
+ // OnInvalidFrame() returns true.
EXPECT_TRUE(adapter->want_write());
- EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor, OnBeforeFrameSent(GOAWAY, 0, _, 0x0));
EXPECT_CALL(visitor,
- OnFrameSent(RST_STREAM, 3, _, 0x0,
+ OnFrameSent(GOAWAY, 0, _, 0x0,
static_cast<int>(Http2ErrorCode::PROTOCOL_ERROR)));
send_result = adapter->Send();
EXPECT_EQ(0, send_result);
- EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::RST_STREAM}));
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::GOAWAY}));
}
-TEST(OgHttp2AdapterServerTest, ServerAllowsNewStreamAboveStreamLimitBeforeAck) {
+TEST(OgHttp2AdapterServerTest,
+ ServerRstStreamsNewStreamAboveStreamLimitBeforeAck) {
DataSavingVisitor visitor;
OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
auto adapter = OgHttp2Adapter::Create(visitor, options);
@@ -3133,8 +3140,8 @@
visitor.Clear();
// Let the client avoid sending a SETTINGS ack and attempt to open more than
- // the advertised number of streams. The overflow stream should be allowed,
- // due to the lack of SETTINGS ack.
+ // the advertised number of streams. The server should still reject the
+ // overflow stream, albeit with RST_STREAM REFUSED_STREAM instead of GOAWAY.
const std::string stream_frames =
TestFrameSequence()
.Headers(1,
@@ -3157,15 +3164,23 @@
EXPECT_CALL(visitor, OnEndHeadersForStream(1));
EXPECT_CALL(visitor, OnEndStream(1));
EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 0x5));
- EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
- EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(4);
- EXPECT_CALL(visitor, OnEndHeadersForStream(3));
- EXPECT_CALL(visitor, OnEndStream(3));
+ EXPECT_CALL(visitor,
+ OnInvalidFrame(
+ 3, Http2VisitorInterface::InvalidFrameError::kRefusedStream));
- // The server should process the bytes without complaint.
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
EXPECT_EQ(static_cast<size_t>(stream_result), stream_frames.size());
- EXPECT_FALSE(adapter->want_write());
+
+ // The server sends a RST_STREAM for the offending stream.
+ EXPECT_TRUE(adapter->want_write());
+ EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0));
+ EXPECT_CALL(visitor,
+ OnFrameSent(RST_STREAM, 3, _, 0x0,
+ static_cast<int>(Http2ErrorCode::REFUSED_STREAM)));
+
+ send_result = adapter->Send();
+ EXPECT_EQ(0, send_result);
+ EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::RST_STREAM}));
}
} // namespace
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 09ea8a5..645a38d 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -1030,16 +1030,24 @@
}
if (stream_map_.size() >= max_inbound_concurrent_streams_) {
- // The new stream would exceed our advertised MAX_CONCURRENT_STREAMS.
- // Currently, use PROTOCOL_ERROR for behavior parity in Envoy.
- // TODO(diannahu): Change to GOAWAY, and add RST_STREAM for exceeding the
- // pending max inbound concurrent streams value.
- EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>(
- stream_id, spdy::ERROR_CODE_PROTOCOL_ERROR));
- const bool ok = visitor_.OnInvalidFrame(
+ // The new stream would exceed our advertised and acknowledged
+ // MAX_CONCURRENT_STREAMS. For parity with nghttp2, treat this error as a
+ // connection-level PROTOCOL_ERROR.
+ visitor_.OnInvalidFrame(
stream_id, Http2VisitorInterface::InvalidFrameError::kProtocol);
+ LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ ConnectionError::kExceededMaxConcurrentStreams);
+ return;
+ }
+ if (stream_map_.size() >= pending_max_inbound_concurrent_streams_) {
+ // The new stream would exceed our advertised but unacked
+ // MAX_CONCURRENT_STREAMS. Refuse the stream for parity with nghttp2.
+ EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>(
+ stream_id, spdy::ERROR_CODE_REFUSED_STREAM));
+ const bool ok = visitor_.OnInvalidFrame(
+ stream_id, Http2VisitorInterface::InvalidFrameError::kRefusedStream);
if (!ok) {
- LatchErrorAndNotify(Http2ErrorCode::PROTOCOL_ERROR,
+ LatchErrorAndNotify(Http2ErrorCode::REFUSED_STREAM,
ConnectionError::kExceededMaxConcurrentStreams);
}
return;
@@ -1206,6 +1214,10 @@
auto settings_ir = absl::make_unique<SpdySettingsIR>();
for (const Http2Setting& setting : settings) {
settings_ir->AddSetting(setting.id, setting.value);
+
+ if (setting.id == Http2KnownSettingsId::MAX_CONCURRENT_STREAMS) {
+ pending_max_inbound_concurrent_streams_ = setting.value;
+ }
}
// Copy the (small) map of settings we are about to send so that we can set
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index 9f00613..670b236 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -410,8 +410,9 @@
// is unlimited, the spec encourages a value of at least 100. We limit
// ourselves to opening 100 until told otherwise by the peer and allow an
// unlimited number from the peer until updated from SETTINGS we send.
- // TODO(diannahu): Add a pending/unacked max inbound concurrent streams value.
uint32_t max_outbound_concurrent_streams_ = 100u;
+ uint32_t pending_max_inbound_concurrent_streams_ =
+ std::numeric_limits<uint32_t>::max();
uint32_t max_inbound_concurrent_streams_ =
std::numeric_limits<uint32_t>::max();
Options options_;
