Skips sending queued frames for streams that have been reset. PiperOrigin-RevId: 421406008
diff --git a/http2/adapter/nghttp2_adapter_test.cc b/http2/adapter/nghttp2_adapter_test.cc index 4e010b1..f177d5f 100644 --- a/http2/adapter/nghttp2_adapter_test.cc +++ b/http2/adapter/nghttp2_adapter_test.cc
@@ -3980,6 +3980,75 @@ EXPECT_FALSE(adapter->want_write()); } +TEST(NgHttp2AdapterTest, SkipsSendingFramesForRejectedStream) { + DataSavingVisitor visitor; + auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor); + + const std::string initial_frames = + TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/true) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + const int64_t initial_result = adapter->ProcessBytes(initial_frames); + EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size()); + + auto body = absl::make_unique<TestDataFrameSource>(visitor, true); + body->AppendPayload("Here is some data, which will be completely ignored!"); + + int submit_result = adapter->SubmitResponse( + 1, ToHeaders({{":status", "200"}}), std::move(body)); + ASSERT_EQ(0, submit_result); + + auto source = absl::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( + {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}}))); + adapter->SubmitMetadata(1, 16384u, std::move(source)); + + adapter->SubmitWindowUpdate(1, 1024); + adapter->SubmitRst(1, Http2ErrorCode::INTERNAL_ERROR); + + // Server initial SETTINGS and SETTINGS ack. + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + // nghttp2 apparently allows extension frames to be sent on reset streams. + EXPECT_CALL(visitor, OnBeforeFrameSent(kMetadataFrameType, 1, _, 0x4)); + EXPECT_CALL(visitor, OnFrameSent(kMetadataFrameType, 1, _, 0x4, 0)); + + // The server sends a RST_STREAM for the offending stream. + // The response HEADERS, DATA and WINDOW_UPDATE are all ignored. + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 1, _, 0x0, + static_cast<int>(Http2ErrorCode::INTERNAL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::INTERNAL_ERROR)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT( + visitor.data(), + EqualsFrames({spdy::SpdyFrameType::SETTINGS, + static_cast<spdy::SpdyFrameType>(kMetadataFrameType), + spdy::SpdyFrameType::RST_STREAM})); +} + TEST(NgHttp2AdapterTest, ServerStartsShutdown) { DataSavingVisitor visitor; auto adapter = NgHttp2Adapter::CreateServerAdapter(visitor);
diff --git a/http2/adapter/oghttp2_adapter_test.cc b/http2/adapter/oghttp2_adapter_test.cc index e9b863c..de3a3e2 100644 --- a/http2/adapter/oghttp2_adapter_test.cc +++ b/http2/adapter/oghttp2_adapter_test.cc
@@ -2258,25 +2258,33 @@ {{HEADER_TABLE_SIZE, 128}, {MAX_FRAME_SIZE, 128 << 10}}); EXPECT_TRUE(adapter_->want_write()); - adapter_->SubmitPriorityForStream(3, 1, 255, true); - adapter_->SubmitRst(3, Http2ErrorCode::CANCEL); + const Http2StreamId accepted_stream = 3; + const Http2StreamId rejected_stream = 7; + adapter_->SubmitPriorityForStream(accepted_stream, 1, 255, true); + adapter_->SubmitRst(rejected_stream, Http2ErrorCode::CANCEL); adapter_->SubmitPing(42); adapter_->SubmitGoAway(13, Http2ErrorCode::HTTP2_NO_ERROR, ""); - adapter_->SubmitWindowUpdate(3, 127); + adapter_->SubmitWindowUpdate(accepted_stream, 127); EXPECT_TRUE(adapter_->want_write()); EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(SETTINGS, 0, _, 0x0)); EXPECT_CALL(http2_visitor_, OnFrameSent(SETTINGS, 0, _, 0x0, 0)); - EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(PRIORITY, 3, _, 0x0)); - EXPECT_CALL(http2_visitor_, OnFrameSent(PRIORITY, 3, _, 0x0, 0)); - EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(RST_STREAM, 3, _, 0x0)); - EXPECT_CALL(http2_visitor_, OnFrameSent(RST_STREAM, 3, _, 0x0, 0x8)); + EXPECT_CALL(http2_visitor_, + OnBeforeFrameSent(PRIORITY, accepted_stream, _, 0x0)); + EXPECT_CALL(http2_visitor_, + OnFrameSent(PRIORITY, accepted_stream, _, 0x0, 0)); + EXPECT_CALL(http2_visitor_, + OnBeforeFrameSent(RST_STREAM, rejected_stream, _, 0x0)); + EXPECT_CALL(http2_visitor_, + OnFrameSent(RST_STREAM, rejected_stream, _, 0x0, 0x8)); EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(PING, 0, _, 0x0)); EXPECT_CALL(http2_visitor_, OnFrameSent(PING, 0, _, 0x0, 0)); EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(GOAWAY, 0, _, 0x0)); EXPECT_CALL(http2_visitor_, OnFrameSent(GOAWAY, 0, _, 0x0, 0)); - EXPECT_CALL(http2_visitor_, OnBeforeFrameSent(WINDOW_UPDATE, 3, _, 0x0)); - EXPECT_CALL(http2_visitor_, OnFrameSent(WINDOW_UPDATE, 3, _, 0x0, 0)); + EXPECT_CALL(http2_visitor_, + OnBeforeFrameSent(WINDOW_UPDATE, accepted_stream, _, 0x0)); + EXPECT_CALL(http2_visitor_, + OnFrameSent(WINDOW_UPDATE, accepted_stream, _, 0x0, 0)); int result = adapter_->Send(); EXPECT_EQ(0, result); @@ -4060,6 +4068,72 @@ EXPECT_FALSE(adapter->want_write()); } +TEST(OgHttp2AdapterServerTest, SkipsSendingFramesForRejectedStream) { + DataSavingVisitor visitor; + OgHttp2Adapter::Options options{.perspective = Perspective::kServer}; + auto adapter = OgHttp2Adapter::Create(visitor, options); + + const std::string initial_frames = + TestFrameSequence() + .ClientPreface() + .Headers(1, + {{":method", "GET"}, + {":scheme", "http"}, + {":authority", "example.com"}, + {":path", "/this/is/request/one"}}, + /*fin=*/true) + .Serialize(); + + testing::InSequence s; + + // Client preface (empty SETTINGS) + EXPECT_CALL(visitor, OnFrameHeader(0, 0, SETTINGS, 0)); + EXPECT_CALL(visitor, OnSettingsStart()); + EXPECT_CALL(visitor, OnSettingsEnd()); + EXPECT_CALL(visitor, OnFrameHeader(1, _, HEADERS, 0x5)); + EXPECT_CALL(visitor, OnBeginHeadersForStream(1)); + EXPECT_CALL(visitor, OnHeaderForStream(1, _, _)).Times(4); + EXPECT_CALL(visitor, OnEndHeadersForStream(1)); + EXPECT_CALL(visitor, OnEndStream(1)); + + const int64_t initial_result = adapter->ProcessBytes(initial_frames); + EXPECT_EQ(static_cast<size_t>(initial_result), initial_frames.size()); + + auto body = absl::make_unique<TestDataFrameSource>(visitor, true); + body->AppendPayload("Here is some data, which will be completely ignored!"); + + int submit_result = adapter->SubmitResponse( + 1, ToHeaders({{":status", "200"}}), std::move(body)); + ASSERT_EQ(0, submit_result); + + auto source = absl::make_unique<TestMetadataSource>(ToHeaderBlock(ToHeaders( + {{"query-cost", "is too darn high"}, {"secret-sauce", "hollandaise"}}))); + adapter->SubmitMetadata(1, 16384u, std::move(source)); + + adapter->SubmitWindowUpdate(1, 1024); + adapter->SubmitRst(1, Http2ErrorCode::INTERNAL_ERROR); + + // Server initial SETTINGS and SETTINGS ack. + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x0)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x0, 0)); + EXPECT_CALL(visitor, OnBeforeFrameSent(SETTINGS, 0, 0, 0x1)); + EXPECT_CALL(visitor, OnFrameSent(SETTINGS, 0, 0, 0x1, 0)); + + // The server sends a RST_STREAM for the offending stream. + // The response HEADERS, DATA and WINDOW_UPDATE are all ignored. + EXPECT_CALL(visitor, OnBeforeFrameSent(RST_STREAM, 1, _, 0x0)); + EXPECT_CALL(visitor, + OnFrameSent(RST_STREAM, 1, _, 0x0, + static_cast<int>(Http2ErrorCode::INTERNAL_ERROR))); + EXPECT_CALL(visitor, OnCloseStream(1, Http2ErrorCode::HTTP2_NO_ERROR)); + + int send_result = adapter->Send(); + EXPECT_EQ(0, send_result); + EXPECT_THAT(visitor.data(), EqualsFrames({spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::SETTINGS, + spdy::SpdyFrameType::RST_STREAM})); +} + TEST(OgHttpAdapterServerTest, ServerStartsShutdown) { DataSavingVisitor visitor; OgHttp2Adapter::Options options{.perspective = Perspective::kServer};
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc index bc8100f..94b728d 100644 --- a/http2/adapter/oghttp2_session.cc +++ b/http2/adapter/oghttp2_session.cc
@@ -520,9 +520,6 @@ } if (frame->frame_type() == spdy::SpdyFrameType::RST_STREAM) { streams_reset_.insert(frame->stream_id()); - } else if (iter != stream_map_.end()) { - // Enqueue RST_STREAM NO_ERROR if appropriate. - r.emplace([this, iter]() { MaybeFinWithRstStream(iter); }); } } if (frame->stream_id() != 0) { @@ -637,6 +634,17 @@ // DATA frames should never be queued. QUICHE_DCHECK_NE(c.frame_type(), 0); + const bool stream_reset = + c.stream_id() != 0 && streams_reset_.count(c.stream_id()) > 0; + if (stream_reset && + c.frame_type() != static_cast<uint8_t>(FrameType::RST_STREAM)) { + // The stream has been reset, so any other remaining frames can be + // skipped. + // TODO(birenroy): inform the visitor of frames that are skipped. + DecrementQueuedFrameCount(c.stream_id(), c.frame_type()); + frames_.pop_front(); + continue; + } // Frames can't accurately report their own length; the actual serialized // length must be used instead. spdy::SpdySerializedFrame frame = framer_.SerializeFrame(*frame_ptr); @@ -695,14 +703,28 @@ } return true; } - auto iter = queued_frames_.find(stream_id); - if (frame_type != FrameType::DATA) { - --iter->second; + + const bool contains_fin = + (frame_type == FrameType::DATA || frame_type == FrameType::HEADERS) && + (flags & 0x01) == 0x01; + auto it = stream_map_.find(stream_id); + const bool still_open_remote = + it != stream_map_.end() && !it->second.half_closed_remote; + if (contains_fin && still_open_remote && + options_.rst_stream_no_error_when_incomplete && + options_.perspective == Perspective::kServer) { + // Since the peer has not yet ended the stream, this endpoint should + // send a RST_STREAM NO_ERROR. See RFC 7540 Section 8.1. + frames_.push_front(absl::make_unique<spdy::SpdyRstStreamIR>( + stream_id, spdy::SpdyErrorCode::ERROR_CODE_NO_ERROR)); + auto queued_result = queued_frames_.insert({stream_id, 1}); + if (!queued_result.second) { + ++(queued_result.first->second); + } + it->second.half_closed_remote = true; } - if (iter->second == 0) { - // TODO(birenroy): Consider passing through `error_code` here. - CloseStreamIfReady(frame_type_int, stream_id); - } + + DecrementQueuedFrameCount(stream_id, frame_type_int); return true; } @@ -715,6 +737,15 @@ return SendResult::SEND_OK; } StreamState& state = it->second; + auto reset_it = streams_reset_.find(stream_id); + if (reset_it != streams_reset_.end()) { + // The stream has been reset; there's no point in sending DATA or trailing + // HEADERS. + state.outbound_body = nullptr; + state.trailers = nullptr; + state.outbound_metadata.clear(); + return SendResult::SEND_OK; + } SendResult connection_can_write = SendResult::SEND_OK; if (!state.outbound_metadata.empty()) { connection_can_write = SendMetadata(stream_id, state.outbound_metadata); @@ -1604,5 +1635,22 @@ } } +void OgHttp2Session::DecrementQueuedFrameCount(uint32_t stream_id, + uint8_t frame_type) { + auto iter = queued_frames_.find(stream_id); + if (iter == queued_frames_.end()) { + QUICHE_LOG(ERROR) << "Unable to find a queued frame count for stream " + << stream_id; + return; + } + if (static_cast<FrameType>(frame_type) != FrameType::DATA) { + --iter->second; + } + if (iter->second == 0) { + // TODO(birenroy): Consider passing through `error_code` here. + CloseStreamIfReady(frame_type, stream_id); + } +} + } // namespace adapter } // namespace http2
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h index eaef9bd..17919f7 100644 --- a/http2/adapter/oghttp2_session.h +++ b/http2/adapter/oghttp2_session.h
@@ -383,6 +383,8 @@ // Handles the potential end of received metadata for the given `stream_id`. void MaybeHandleMetadataEndForStream(Http2StreamId stream_id); + void DecrementQueuedFrameCount(uint32_t stream_id, uint8_t frame_type); + // Receives events when inbound frames are parsed. Http2VisitorInterface& visitor_;