Remove stream state from OgHttp2Session once a stream is closed.
This CL creates OgHttp2Session::CloseStream(), which calls the visitor's
OnCloseStream() and removes the stream state from OgHttp2Session's internal
map. This completes a TODO in its own right and also prepares for the change to
check the size of the internal map against the peer's advertised
SETTINGS_MAX_CONCURRENT_STREAMS value when deciding whether to create a stream
immediately or queue it for later.
PiperOrigin-RevId: 398512884
diff --git a/http2/adapter/oghttp2_session.cc b/http2/adapter/oghttp2_session.cc
index d7ce5e0..0ec920e 100644
--- a/http2/adapter/oghttp2_session.cc
+++ b/http2/adapter/oghttp2_session.cc
@@ -5,6 +5,7 @@
#include "absl/memory/memory.h"
#include "absl/strings/escaping.h"
+#include "http2/adapter/http2_protocol.h"
#include "http2/adapter/oghttp2_util.h"
#include "spdy/core/spdy_protocol.h"
@@ -342,7 +343,7 @@
// If this endpoint is resetting the stream, the stream should be
// closed. This endpoint is already aware of the outbound RST_STREAM and
// its error code, so close with NO_ERROR.
- visitor_.OnCloseStream(c.stream_id(), Http2ErrorCode::NO_ERROR);
+ CloseStream(c.stream_id(), Http2ErrorCode::NO_ERROR);
}
frames_.pop_front();
@@ -397,7 +398,7 @@
break;
} else if (length == DataFrameSource::kError) {
source_can_produce = false;
- visitor_.OnCloseStream(stream_id, Http2ErrorCode::INTERNAL_ERROR);
+ CloseStream(stream_id, Http2ErrorCode::INTERNAL_ERROR);
break;
}
const bool fin = end_data ? state.outbound_body->send_fin() : false;
@@ -501,10 +502,8 @@
const bool end_stream = data_source == nullptr;
if (end_stream) {
if (iter->second.half_closed_remote) {
- visitor_.OnCloseStream(stream_id, Http2ErrorCode::NO_ERROR);
+ CloseStream(stream_id, Http2ErrorCode::NO_ERROR);
}
- // TODO(birenroy): the server adapter should probably delete stream state
- // when calling visitor_.OnCloseStream.
} else {
// Add data source to stream state
iter->second.outbound_body = std::move(data_source);
@@ -607,7 +606,7 @@
options_.perspective == Perspective::kClient) {
// From the client's perspective, the stream can be closed if it's already
// half_closed_local.
- visitor_.OnCloseStream(stream_id, Http2ErrorCode::NO_ERROR);
+ CloseStream(stream_id, Http2ErrorCode::NO_ERROR);
}
}
@@ -642,9 +641,7 @@
write_scheduler_.UnregisterStream(stream_id);
}
visitor_.OnRstStream(stream_id, TranslateErrorCode(error_code));
- // TODO(birenroy): Consider bundling "close stream" behavior into a dedicated
- // method that also cleans up the stream map.
- visitor_.OnCloseStream(stream_id, TranslateErrorCode(error_code));
+ CloseStream(stream_id, TranslateErrorCode(error_code));
}
void OgHttp2Session::OnSettings() {
@@ -841,7 +838,7 @@
state.half_closed_local = true;
if (options_.perspective == Perspective::kServer) {
if (state.half_closed_remote) {
- visitor_.OnCloseStream(stream_id, Http2ErrorCode::NO_ERROR);
+ CloseStream(stream_id, Http2ErrorCode::NO_ERROR);
} else {
// Since the peer has not yet ended the stream, this endpoint should
// send a RST_STREAM NO_ERROR. See RFC 7540 Section 8.1.
@@ -849,8 +846,6 @@
stream_id, spdy::SpdyErrorCode::ERROR_CODE_NO_ERROR));
// Enqueuing the RST_STREAM also invokes OnCloseStream.
}
- // TODO(birenroy): the server adapter should probably delete stream state
- // when calling visitor_.OnCloseStream.
}
}
@@ -897,5 +892,11 @@
SendHeaders(stream_id, std::move(headers), end_stream);
}
+void OgHttp2Session::CloseStream(Http2StreamId stream_id,
+ Http2ErrorCode error_code) {
+ visitor_.OnCloseStream(stream_id, error_code);
+ stream_map_.erase(stream_id);
+}
+
} // namespace adapter
} // namespace http2
diff --git a/http2/adapter/oghttp2_session.h b/http2/adapter/oghttp2_session.h
index d5973a5..cd9bbda 100644
--- a/http2/adapter/oghttp2_session.h
+++ b/http2/adapter/oghttp2_session.h
@@ -243,6 +243,9 @@
std::unique_ptr<DataFrameSource> data_source,
void* user_data);
+ // Closes the given `stream_id` with the given `error_code`.
+ void CloseStream(Http2StreamId stream_id, Http2ErrorCode error_code);
+
// Receives events when inbound frames are parsed.
Http2VisitorInterface& visitor_;
diff --git a/http2/adapter/oghttp2_session_test.cc b/http2/adapter/oghttp2_session_test.cc
index 8e088ee..4fdee88 100644
--- a/http2/adapter/oghttp2_session_test.cc
+++ b/http2/adapter/oghttp2_session_test.cc
@@ -588,11 +588,10 @@
EXPECT_GT(session.GetHpackDecoderDynamicTableSize(), 0);
- // TODO(birenroy): drop stream state when streams are closed. It should no
- // longer be possible to set user data.
+ // It should no longer be possible to set user data on a closed stream.
const char* kSentinel3 = "another arbitrary pointer";
session.SetStreamUserData(3, const_cast<char*>(kSentinel3));
- EXPECT_EQ(kSentinel3, session.GetStreamUserData(3));
+ EXPECT_EQ(nullptr, session.GetStreamUserData(3));
EXPECT_EQ(session.GetRemoteWindowSize(),
kInitialFlowControlWindowSize + 1000);
