filter state: simplify the filter state construction (envoyproxy#33287)

---------

Signed-off-by: wbpcode <wbphub@live.com>

source/common/http/conn_manager_impl.cc

@@ -794,14 +794,14 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
  : makeOptRef<const TracingConnectionManagerConfig>(
  *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_(

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)) {

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<FilterStateImpl>(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<FilterStateImpl>(std::move(ancestor), parent_life_span);
+}
+
 void FilterStateImpl::setData(absl::string_view data_name, std::shared_ptr<Object> 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_ptr<Objec
  "conflicting life_span on the same data_name.");
  return;
  }
- maybeCreateParent(ParentAccessMode::ReadWrite);
+ // Note if ancestor argument of ctor is not nullptr, parent will be created at the time of
+ // construction directly and this call will be a no-op.
+ // So we only need to consider the case where ancestor is nullptr.
+ maybeCreateParent(nullptr);
  parent_->setData(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<FilterStateSharedPtr>(ancestor_)) {
- FilterStateSharedPtr ancestor = absl::get<FilterStateSharedPtr>(ancestor_);
- if (ancestor == nullptr || ancestor->lifeSpan() != life_span_ + 1) {
- parent_ = std::make_shared<FilterStateImpl>(ancestor, FilterState::LifeSpan(life_span_ + 1));
- } else {
- parent_ = ancestor;
- }
- return;
- }
-
- auto lazy_create_ancestor = absl::get<LazyCreateAncestor>(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<FilterStateImpl>(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<FilterStateImpl>(FilterState::LifeSpan(life_span_ + 1));
- }
- parent_ = lazy_create_ancestor.first;
-}
-
 } // namespace StreamInfo
 } // namespace Envoy

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<FilterStateSharedPtr, FilterState::LifeSpan>;
- /**
- * @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<FilterStateSharedPtr, LazyCreateAncestor> ancestor_;
  FilterStateSharedPtr parent_;
  const FilterState::LifeSpan life_span_;
  absl::flat_hash_map<std::string, std::unique_ptr<FilterObject>> data_storage_;

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<FilterStateImpl>(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<FilterStateImpl>(life_span)) {}
+ FilterState::LifeSpan life_span, FilterStateSharedPtr ancestor_filter_state = nullptr)
+ : StreamInfoImpl(
+ absl::nullopt, time_source, downstream_connection_info_provider,
+ std::make_shared<FilterStateImpl>(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>(FilterStateImpl::LazyCreateAncestor(
- std::move(parent_filter_state), parent_life_span),
- life_span)) {}
+ std::make_shared<FilterStateImpl>(std::move(ancestor_filter_state), life_span)) {}
 
  StreamInfoImpl(
  absl::optional<Http::Protocol> protocol, TimeSource& time_source,

test/common/http/filter_manager_test.cc

@@ -35,8 +35,7 @@ class FilterManagerTest : public testing::Test {
  void initialize() {
  filter_manager_ = std::make_unique<DownstreamFilterManager>(
  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.

test/mocks/network/connection.cc

@@ -114,6 +114,9 @@ template <class T> 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::FilterStateImpl>(StreamInfo::FilterState::LifeSpan::Connection);
+
  initializeMockConnection(*this);
 }
 MockConnection::~MockConnection() = default;