Avoid OnInvalidFrame() to make content-length errors stream-level in oghttp2.
As background, cl/422597021 made oghttp2 follow nghttp2 on calling the visitor
OnInvalidFrame() to make an integration test pass with a connection-level error
expectation [1], but another test expects a stream-level error instead [2].
These tests both pass with nghttp2 because nghttp2 handles content-length
differently based on whether the end_stream frame is headers/trailers vs. data
(see cl/422597021 description for details). However, preferred behavior is for
content-length errors to be stream-level across the board.
This CL updates oghttp2 such that content-length errors do not call
OnInvalidFrame() and are thus stream-level, for both tests. The expectations in
[1] will be changed to align with HTTP/3.
[1]
http://google3/third_party/envoy/src/test/integration/multiplexed_integration_test.cc;l=1972;rcl=414009764
[2]
http://google3/third_party/envoy/src/test/integration/protocol_integration_test.cc;l=3535;rcl=421694486
PiperOrigin-RevId: 422833845
diff --git a/http2/adapter/http2_util.cc b/http2/adapter/http2_util.cc
index f93b161..553c0bd 100644
--- a/http2/adapter/http2_util.cc
+++ b/http2/adapter/http2_util.cc
@@ -97,8 +97,6 @@
return "InvalidPushPromise";
case ConnectionError::kExceededMaxConcurrentStreams:
return "ExceededMaxConcurrentStreams";
- case ConnectionError::kHttpMessaging:
- return "HttpMessaging";
}
return "UnknownConnectionError";
}
diff --git a/http2/adapter/http2_visitor_interface.h b/http2/adapter/http2_visitor_interface.h
index 61c76b7..0e8619f 100644
--- a/http2/adapter/http2_visitor_interface.h
+++ b/http2/adapter/http2_visitor_interface.h
@@ -78,9 +78,6 @@
kInvalidPushPromise,
// The peer exceeded the max concurrent streams limit.
kExceededMaxConcurrentStreams,
- // The peer violated HTTP messaging semantics.
- // TODO(b/214614028): Remove once these errors can be just RST_STREAM'd.
- kHttpMessaging,
};
virtual void OnConnectionError(ConnectionError error) = 0;
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc
index 3203560..3660d20 100644
--- a/http2/adapter/oghttp2_adapter_test.cc
+++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -4572,8 +4572,8 @@
EXPECT_CALL(visitor, OnSettingsEnd());
// Stream 1: content-length is larger than actual data
- // All data and then OnInvalidFrame() are delivered to the visitor. Note that
- // nghttp2 does not deliver OnInvalidFrame().
+ // All data is delivered to the visitor. Note that neither oghttp2 nor
+ // nghttp2 delivers OnInvalidFrame().
EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 4));
EXPECT_CALL(visitor, OnBeginHeadersForStream(1));
EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(5);
@@ -4581,33 +4581,24 @@
EXPECT_CALL(visitor, OnFrameHeader(1, _, DATA, 1));
EXPECT_CALL(visitor, OnBeginDataForStream(1, 1));
EXPECT_CALL(visitor, OnDataForStream(1, "h"));
- EXPECT_CALL(visitor,
- OnInvalidFrame(
- 1, Http2VisitorInterface::InvalidFrameError::kHttpMessaging));
// Stream 3: content-length is smaller than actual data
// The beginning of data is delivered to the visitor, but not the actual data.
- // OnInvalidFrame() is delivered (unlike with nghttp2).
+ // Again, neither oghttp2 nor nghttp2 delivers OnInvalidFrame().
EXPECT_CALL(visitor, OnFrameHeader(3, _, HEADERS, 4));
EXPECT_CALL(visitor, OnBeginHeadersForStream(3));
EXPECT_CALL(visitor, OnHeaderForStream(3, _, _)).Times(5);
EXPECT_CALL(visitor, OnEndHeadersForStream(3));
EXPECT_CALL(visitor, OnFrameHeader(3, _, DATA, 1));
EXPECT_CALL(visitor, OnBeginDataForStream(3, 5));
- EXPECT_CALL(visitor,
- OnInvalidFrame(
- 3, Http2VisitorInterface::InvalidFrameError::kHttpMessaging));
// Stream 5: content-length is invalid and HEADERS ends the stream
- // Only oghttp2 invokes OnEndHeadersForStream(), but both oghttp2 and nghttp2
- // invoke OnInvalidFrame().
+ // Only oghttp2 invokes OnEndHeadersForStream(). Only nghttp2 invokes
+ // OnInvalidFrame().
EXPECT_CALL(visitor, OnFrameHeader(5, _, HEADERS, 5));
EXPECT_CALL(visitor, OnBeginHeadersForStream(5));
EXPECT_CALL(visitor, OnHeaderForStream(5, _, _)).Times(5);
EXPECT_CALL(visitor, OnEndHeadersForStream(5));
- EXPECT_CALL(visitor,
- OnInvalidFrame(
- 5, Http2VisitorInterface::InvalidFrameError::kHttpMessaging));
const int64_t stream_result = adapter->ProcessBytes(stream_frames);
EXPECT_EQ(stream_frames.size(), static_cast<size_t>(stream_result));
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index 59ce29b..827e80b 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -1701,15 +1701,6 @@
void OgHttp2Session::HandleContentLengthError(Http2StreamId stream_id) {
EnqueueFrame(absl::make_unique<spdy::SpdyRstStreamIR>(
stream_id, spdy::ERROR_CODE_PROTOCOL_ERROR));
- // TODO(b/214614028): The OnInvalidFrame() is for nghttp2 parity, but parity
- // causes this error to be connection-level in Envoy. Revisit after migration.
- const bool ok = visitor_.OnInvalidFrame(
- stream_id, Http2VisitorInterface::InvalidFrameError::kHttpMessaging);
- if (!ok) {
- fatal_visitor_callback_failure_ = true;
- LatchErrorAndNotify(Http2ErrorCode::REFUSED_STREAM,
- ConnectionError::kHttpMessaging);
- }
}
} // namespace adapter
