diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 311f32ceaeb9..75d3a062bf19 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -794,14 +794,14 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect : makeOptRef( *connection_manager_.config_->tracingConfig())), stream_id_(connection_manager.random_generator_.random()), - filter_manager_( - *this, *connection_manager_.dispatcher_, - connection_manager_.read_callbacks_->connection(), stream_id_, std::move(account), - connection_manager_.config_->proxy100Continue(), buffer_limit, - connection_manager_.config_->filterFactory(), connection_manager_.config_->localReply(), - connection_manager_.codec_->protocol(), connection_manager_.timeSource(), - connection_manager_.read_callbacks_->connection().streamInfo().filterState(), - StreamInfo::FilterState::LifeSpan::Connection, connection_manager_.overload_manager_), + filter_manager_(*this, *connection_manager_.dispatcher_, + connection_manager_.read_callbacks_->connection(), stream_id_, + std::move(account), connection_manager_.config_->proxy100Continue(), + buffer_limit, connection_manager_.config_->filterFactory(), + connection_manager_.config_->localReply(), + connection_manager_.codec_->protocol(), connection_manager_.timeSource(), + connection_manager_.read_callbacks_->connection().streamInfo().filterState(), + connection_manager_.overload_manager_), request_response_timespan_(new Stats::HistogramCompletableTimespanImpl( connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), header_validator_( diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 2078cc8a11a0..2952dcc07071 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -1101,13 +1101,12 @@ class DownstreamFilterManager : public FilterManager { const LocalReply::LocalReply& local_reply, Http::Protocol protocol, TimeSource& time_source, StreamInfo::FilterStateSharedPtr parent_filter_state, - StreamInfo::FilterState::LifeSpan filter_state_life_span, Server::OverloadManager& overload_manager) : FilterManager(filter_manager_callbacks, dispatcher, connection, stream_id, account, proxy_100_continue, buffer_limit, filter_chain_factory), stream_info_(protocol, time_source, connection.connectionInfoProviderSharedPtr(), - parent_filter_state, filter_state_life_span, - StreamInfo::FilterState::LifeSpan::FilterChain), + StreamInfo::FilterState::LifeSpan::FilterChain, + std::move(parent_filter_state)), local_reply_(local_reply), downstream_filter_load_shed_point_(overload_manager.getLoadShedPoint( Server::LoadShedPointName::get().HttpDownstreamFilterCheck)) { diff --git a/source/common/stream_info/filter_state_impl.cc b/source/common/stream_info/filter_state_impl.cc index e59e1138cdaa..1ceb96a22ff8 100644 --- a/source/common/stream_info/filter_state_impl.cc +++ b/source/common/stream_info/filter_state_impl.cc @@ -5,6 +5,32 @@ namespace Envoy { namespace StreamInfo { +void FilterStateImpl::maybeCreateParent(FilterStateSharedPtr ancestor) { + // If we already have a parent, or we're at the top span, we don't need to create + // a parent. + if (parent_ != nullptr || life_span_ >= FilterState::LifeSpan::TopSpan) { + return; + } + + const auto parent_life_span = FilterState::LifeSpan(life_span_ + 1); + + // No ancestor, or the provided ancestor has a shorter life span than the parent + // we need to create, so we create a new parent. + if (ancestor == nullptr || ancestor->lifeSpan() < parent_life_span) { + parent_ = std::make_shared(parent_life_span); + return; + } + + // The ancestor is our immediate parent, use it. + if (ancestor->lifeSpan() == parent_life_span) { + parent_ = std::move(ancestor); + return; + } + + // The ancestor is not our immediate parent, so we need to create a chain of parents. + parent_ = std::make_shared(std::move(ancestor), parent_life_span); +} + void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr data, FilterState::StateType state_type, FilterState::LifeSpan life_span, StreamSharingMayImpactPooling stream_sharing) { @@ -14,7 +40,10 @@ void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptrsetData(data_name, data, state_type, life_span, stream_sharing); return; } @@ -123,43 +152,5 @@ bool FilterStateImpl::hasDataWithNameInternally(absl::string_view data_name) con return data_storage_.contains(data_name); } -void FilterStateImpl::maybeCreateParent(ParentAccessMode parent_access_mode) { - if (parent_ != nullptr) { - return; - } - if (life_span_ >= FilterState::LifeSpan::TopSpan) { - return; - } - if (absl::holds_alternative(ancestor_)) { - FilterStateSharedPtr ancestor = absl::get(ancestor_); - if (ancestor == nullptr || ancestor->lifeSpan() != life_span_ + 1) { - parent_ = std::make_shared(ancestor, FilterState::LifeSpan(life_span_ + 1)); - } else { - parent_ = ancestor; - } - return; - } - - auto lazy_create_ancestor = absl::get(ancestor_); - // If we're only going to read data from our parent, we don't need to create lazy ancestor, - // because they're empty anyways. - if (parent_access_mode == ParentAccessMode::ReadOnly && lazy_create_ancestor.first == nullptr) { - return; - } - - // Lazy ancestor is not our immediate parent. - if (lazy_create_ancestor.second != life_span_ + 1) { - parent_ = std::make_shared(lazy_create_ancestor, - FilterState::LifeSpan(life_span_ + 1)); - return; - } - // Lazy parent is our immediate parent. - if (lazy_create_ancestor.first == nullptr) { - lazy_create_ancestor.first = - std::make_shared(FilterState::LifeSpan(life_span_ + 1)); - } - parent_ = lazy_create_ancestor.first; -} - } // namespace StreamInfo } // namespace Envoy diff --git a/source/common/stream_info/filter_state_impl.h b/source/common/stream_info/filter_state_impl.h index b6c736ffc847..47390432719a 100644 --- a/source/common/stream_info/filter_state_impl.h +++ b/source/common/stream_info/filter_state_impl.h @@ -14,29 +14,22 @@ namespace StreamInfo { class FilterStateImpl : public FilterState { public: - FilterStateImpl(FilterState::LifeSpan life_span) : life_span_(life_span) { - maybeCreateParent(ParentAccessMode::ReadOnly); - } + FilterStateImpl(FilterState::LifeSpan life_span) : life_span_(life_span) {} /** - * @param ancestor a std::shared_ptr storing an already created ancestor. + * @param ancestor a std::shared_ptr storing an already created ancestor. If ancestor is + * nullptr then the parent will be created lazily. + * NOTE: ancestor may be the parent or a grandparent or even further up of the chain. It + * may be different from the immediate parent. * @param life_span the life span this is handling. */ FilterStateImpl(FilterStateSharedPtr ancestor, FilterState::LifeSpan life_span) - : ancestor_(ancestor), life_span_(life_span) { - maybeCreateParent(ParentAccessMode::ReadOnly); - } - - using LazyCreateAncestor = std::pair; - /** - * @param ancestor a std::pair storing an ancestor, that can be passed in as a way to lazy - * initialize a FilterState that's owned by an object with bigger scope than this. This is to - * avoid creating a FilterState that's empty in most cases. - * @param life_span the life span this is handling. - */ - FilterStateImpl(LazyCreateAncestor lazy_create_ancestor, FilterState::LifeSpan life_span) - : ancestor_(lazy_create_ancestor), life_span_(life_span) { - maybeCreateParent(ParentAccessMode::ReadOnly); + : life_span_(life_span) { + // If ancestor is nullptr, we will create the parent lazily, otherwise we will create + // the parent immediately. + if (ancestor != nullptr) { + maybeCreateParent(std::move(ancestor)); + } } // FilterState @@ -57,10 +50,8 @@ class FilterStateImpl : public FilterState { private: // This only checks the local data_storage_ for data_name existence. bool hasDataWithNameInternally(absl::string_view data_name) const; - enum class ParentAccessMode { ReadOnly, ReadWrite }; - void maybeCreateParent(ParentAccessMode parent_access_mode); + void maybeCreateParent(FilterStateSharedPtr ancestor); - absl::variant ancestor_; FilterStateSharedPtr parent_; const FilterState::LifeSpan life_span_; absl::flat_hash_map> data_storage_; diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 833f57444a19..5fbc6b7f7e14 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -111,27 +111,18 @@ struct StreamInfoImpl : public StreamInfo { StreamInfoImpl( TimeSource& time_source, const Network::ConnectionInfoProviderSharedPtr& downstream_connection_info_provider, - FilterState::LifeSpan life_span) - : StreamInfoImpl(absl::nullopt, time_source, downstream_connection_info_provider, - std::make_shared(life_span)) {} - - StreamInfoImpl( - Http::Protocol protocol, TimeSource& time_source, - const Network::ConnectionInfoProviderSharedPtr& downstream_connection_info_provider, - FilterState::LifeSpan life_span) - : StreamInfoImpl(protocol, time_source, downstream_connection_info_provider, - std::make_shared(life_span)) {} + FilterState::LifeSpan life_span, FilterStateSharedPtr ancestor_filter_state = nullptr) + : StreamInfoImpl( + absl::nullopt, time_source, downstream_connection_info_provider, + std::make_shared(std::move(ancestor_filter_state), life_span)) {} StreamInfoImpl( Http::Protocol protocol, TimeSource& time_source, const Network::ConnectionInfoProviderSharedPtr& downstream_connection_info_provider, - FilterStateSharedPtr parent_filter_state, FilterState::LifeSpan parent_life_span, - FilterState::LifeSpan life_span) + FilterState::LifeSpan life_span, FilterStateSharedPtr ancestor_filter_state = nullptr) : StreamInfoImpl( protocol, time_source, downstream_connection_info_provider, - std::make_shared(FilterStateImpl::LazyCreateAncestor( - std::move(parent_filter_state), parent_life_span), - life_span)) {} + std::make_shared(std::move(ancestor_filter_state), life_span)) {} StreamInfoImpl( absl::optional protocol, TimeSource& time_source, diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc index 4afeb7f831e6..8d8e605f86e0 100644 --- a/test/common/http/filter_manager_test.cc +++ b/test/common/http/filter_manager_test.cc @@ -35,8 +35,7 @@ class FilterManagerTest : public testing::Test { void initialize() { filter_manager_ = std::make_unique( filter_manager_callbacks_, dispatcher_, connection_, 0, nullptr, true, 10000, - filter_factory_, local_reply_, protocol_, time_source_, filter_state_, - StreamInfo::FilterState::LifeSpan::Connection, overload_manager_); + filter_factory_, local_reply_, protocol_, time_source_, filter_state_, overload_manager_); } // Simple helper to wrapper filter to the factory function. diff --git a/test/mocks/network/connection.cc b/test/mocks/network/connection.cc index 2e88faa3b697..75bce5b8a59c 100644 --- a/test/mocks/network/connection.cc +++ b/test/mocks/network/connection.cc @@ -114,6 +114,9 @@ template static void initializeMockConnection(T& connection) { MockConnection::MockConnection() { stream_info_.downstream_connection_info_provider_->setRemoteAddress( Utility::resolveUrl("tcp://10.0.0.3:50000")); + stream_info_.filter_state_ = + std::make_shared(StreamInfo::FilterState::LifeSpan::Connection); + initializeMockConnection(*this); } MockConnection::~MockConnection() = default;