envoyproxy · ggreenway · Jan 19, 2021 · Jan 6, 2021 · Jan 7, 2021 · Jan 7, 2021
diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst
@@ -68,6 +68,7 @@ New Features
 * compression: the :ref:`compressor <envoy_v3_api_msg_extensions.filters.http.compressor.v3.Compressor>` filter adds support for compressing request payloads. Its configuration is unified with the :ref:`decompressor <envoy_v3_api_msg_extensions.filters.http.decompressor.v3.Decompressor>` filter with two new fields for different directions - :ref:`requests <envoy_v3_api_field_extensions.filters.http.compressor.v3.Compressor.request_direction_config>` and :ref:`responses <envoy_v3_api_field_extensions.filters.http.compressor.v3.Compressor.response_direction_config>`. The latter deprecates the old response-specific fields and, if used, roots the response-specific stats in `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.response.*` instead of `<stat_prefix>.compressor.<compressor_library.name>.<compressor_library_stat_prefix>.*`.
 * config: added ability to flush stats when the admin's :ref:`/stats endpoint <operations_admin_interface_stats>` is hit instead of on a timer via :ref:`stats_flush_on_admin <envoy_v3_api_field_config.bootstrap.v3.Bootstrap.stats_flush_on_admin>`.
 * config: added new runtime feature `envoy.features.enable_all_deprecated_features` that allows the use of all deprecated features.
+* dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow us to dump more debug information on crash.
 * formatter: added new :ref:`text_format_source <envoy_v3_api_field_config.core.v3.SubstitutionFormatString.text_format_source>` field to support format strings both inline and from a file.
 * grpc: implemented header value syntax support when defining :ref:`initial metadata <envoy_v3_api_field_config.core.v3.GrpcService.initial_metadata>` for gRPC-based `ext_authz` :ref:`HTTP <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>` and :ref:`network <envoy_v3_api_field_extensions.filters.network.ext_authz.v3.ExtAuthz.grpc_service>` filters, and :ref:`ratelimit <envoy_v3_api_field_config.ratelimit.v3.RateLimitServiceConfig.grpc_service>` filters.
 * grpc-json: added support for configuring :ref:`unescaping behavior <envoy_v3_api_field_extensions.filters.http.grpc_json_transcoder.v3.GrpcJsonTranscoder.url_unescape_spec>` for path components.

diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h
@@ -86,15 +86,18 @@ class DispatcherBase {
   virtual Event::SchedulableCallbackPtr createSchedulableCallback(std::function<void()> cb) PURE;
 
   /**
-   * Sets a tracked object, which is currently operating in this Dispatcher.
-   * This should be cleared with another call to setTrackedObject() when the object is done doing
-   * work. Calling setTrackedObject(nullptr) results in no object being tracked.
+   * Appends a tracked object to the current stack of tracked objects operating
+   * in the dispatcher.
    *
-   * This is optimized for performance, to avoid allocation where we do scoped object tracking.
-   *
-   * @return The previously tracked object or nullptr if there was none.
+   * It's recommended to use ScopeTrackerScopeState to manage the object's tracking. If directly
+   * invoking, there needs to be a subsequent call to popTrackedObject().
+   */
+  virtual void appendTrackedObject(const ScopeTrackedObject* object) PURE;
+
+  /**
+   * Removes the top of the stack of tracked object and asserts that it was expected.
    */
-  virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE;
+  virtual void popTrackedObject(const ScopeTrackedObject* expected_object) PURE;
 
   /**
    * Validates that an operation is thread-safe with respect to this dispatcher; i.e. that the

diff --git a/include/envoy/server/fatal_action_config.h b/include/envoy/server/fatal_action_config.h
@@ -1,6 +1,7 @@
 #pragma once
 
 #include <memory>
+#include <vector>
 
 #include "envoy/common/pure.h"
 #include "envoy/config/bootstrap/v3/bootstrap.pb.h"
@@ -18,10 +19,9 @@ class FatalAction {
   virtual ~FatalAction() = default;
   /**
    * Callback function to run when we are crashing.
-   * @param current_object the object we were working on when we started
-   * crashing.
+   * @param objects a vector of objects we were working on when we started crashing.
    */
-  virtual void run(const ScopeTrackedObject* current_object) PURE;
+  virtual void run(const std::vector<const ScopeTrackedObject*>& tracked_objects) PURE;
 
   /**
    * @return whether the action is async-signal-safe.

diff --git a/source/common/common/scope_tracker.h b/source/common/common/scope_tracker.h
@@ -3,24 +3,35 @@
 #include "envoy/common/scope_tracker.h"
 #include "envoy/event/dispatcher.h"
 
+#include "common/common/assert.h"
+
 namespace Envoy {
 
-// A small class for tracking the scope of the object which is currently having
+// A small class for tracking the managing the scope of a tracked object which is currently having
 // work done in this thread.
 //
-// When created, it sets the tracked object in the dispatcher, and when destroyed it points the
-// dispatcher at the previously tracked object.
+// When created, it appends the tracked object to the dispatcher's stack of tracked objects, and
+// when destroyed it pops the dispatcher's stack of tracked object, which should be the object it
+// registered.
 class ScopeTrackerScopeState {
 public:
   ScopeTrackerScopeState(const ScopeTrackedObject* object, Event::Dispatcher& dispatcher)
-      : dispatcher_(dispatcher) {
-    latched_object_ = dispatcher_.setTrackedObject(object);
+      : registered_object_(object), dispatcher_(dispatcher) {
+    dispatcher_.appendTrackedObject(registered_object_);
+  }
+
+  ~ScopeTrackerScopeState() {
+    // If ScopeTrackerScopeState is always used for managing tracked objects,
+    // then the object popped off should be the object we registered.
+    dispatcher_.popTrackedObject(registered_object_);
   }
 
-  ~ScopeTrackerScopeState() { dispatcher_.setTrackedObject(latched_object_); }
+  // Make this object stack-only, it doesn't make sense for it
+  // to be on the heap since it's tracking a stack of active operations.
+  void* operator new(std::size_t) = delete;
 
 private:
-  const ScopeTrackedObject* latched_object_;
+  const ScopeTrackedObject* registered_object_;
   Event::Dispatcher& dispatcher_;
 };
 

diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc
@@ -7,10 +7,12 @@
 #include <vector>
 
 #include "envoy/api/api.h"
+#include "envoy/common/scope_tracker.h"
 #include "envoy/network/listen_socket.h"
 #include "envoy/network/listener.h"
 
 #include "common/buffer/buffer_impl.h"
+#include "common/common/assert.h"
 #include "common/common/lock_guard.h"
 #include "common/common/thread.h"
 #include "common/event/file_event_impl.h"
@@ -36,6 +38,12 @@
 
 namespace Envoy {
 namespace Event {
+namespace {
+// Our tracked object stack likely won't grow larger than this initial
+// reservation; this should make appends constant time since we shouldn't
+// have to grow the stack larger.
+constexpr size_t ExpectedMaxTrackedObjectStackDepth = 10;
+} // namespace
 
 DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api,
                                Event::TimeSystem& time_system,
@@ -49,6 +57,7 @@ DispatcherImpl::DispatcherImpl(const std::string& name, Api::Api& api,
       post_cb_(base_scheduler_.createSchedulableCallback([this]() -> void { runPostCallbacks(); })),
       current_to_delete_(&to_delete_1_) {
   ASSERT(!name_.empty());
+  tracked_object_stack_.reserve(ExpectedMaxTrackedObjectStackDepth);
   FatalErrorHandler::registerFatalErrorHandler(*this);
   updateApproximateMonotonicTimeInternal();
   base_scheduler_.registerOnPrepareCallback(
@@ -287,6 +296,16 @@ void DispatcherImpl::runPostCallbacks() {
   }
 }
 
+void DispatcherImpl::onFatalError(std::ostream& os) const {
+  // Dump the state of the tracked objects in the dispatcher if thread safe. This generally
+  // results in dumping the active state only for the thread which caused the fatal error.
+  if (isThreadSafe()) {
+    for (auto iter = tracked_object_stack_.rbegin(); iter != tracked_object_stack_.rend(); ++iter) {
+      (*iter)->dumpState(os);
+    }
+  }
+}
+
 void DispatcherImpl::runFatalActionsOnTrackedObject(
     const FatalAction::FatalActionPtrList& actions) const {
   // Only run if this is the dispatcher of the current thread and
@@ -296,7 +315,7 @@ void DispatcherImpl::runFatalActionsOnTrackedObject(
   }
 
   for (const auto& action : actions) {
-    action->run(current_object_);
+    action->run(tracked_object_stack_);
   }
 }
 
@@ -306,5 +325,22 @@ void DispatcherImpl::touchWatchdog() {
   }
 }
 
+void DispatcherImpl::appendTrackedObject(const ScopeTrackedObject* object) {
+  tracked_object_stack_.push_back(object);
+}
+
+void DispatcherImpl::popTrackedObject(const ScopeTrackedObject* expected_object) {
+  if (tracked_object_stack_.empty()) {
+    ASSERT(!expected_object,
+           "Tracked object stack is empty yet we expected a non-null tracked object on the top.");
+    return;
+  }
+
+  const ScopeTrackedObject* top = tracked_object_stack_.back();
+  tracked_object_stack_.pop_back();
+  ASSERT(top == expected_object,
+         "Popped the top of the tracked object stack, but it wasn't the expected object!");
+}
+
 } // namespace Event
 } // namespace Envoy
diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h
@@ -74,25 +74,13 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>,
   void post(std::function<void()> callback) override;
   void run(RunType type) override;
   Buffer::WatermarkFactory& getWatermarkFactory() override { return *buffer_factory_; }
-  const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) override {
-    const ScopeTrackedObject* return_object = current_object_;
-    current_object_ = object;
-    return return_object;
-  }
+  void appendTrackedObject(const ScopeTrackedObject* object) override;
+  void popTrackedObject(const ScopeTrackedObject* expected_object) override;
   MonotonicTime approximateMonotonicTime() const override;
   void updateApproximateMonotonicTime() override;
 
   // FatalErrorInterface
-  void onFatalError(std::ostream& os) const override {
-    // Dump the state of the tracked object if it is in the current thread. This generally results
-    // in dumping the active state only for the thread which caused the fatal error.
-    if (isThreadSafe()) {
-      if (current_object_) {
-        current_object_->dumpState(os);
-      }
-    }
-  }
-
+  void onFatalError(std::ostream& os) const override;
   void
   runFatalActionsOnTrackedObject(const FatalAction::FatalActionPtrList& actions) const override;
 
@@ -150,7 +138,7 @@ class DispatcherImpl : Logger::Loggable<Logger::Id::main>,
   std::vector<DeferredDeletablePtr>* current_to_delete_;
   Thread::MutexBasicLockable post_lock_;
   std::list<std::function<void()>> post_callbacks_ ABSL_GUARDED_BY(post_lock_);
-  const ScopeTrackedObject* current_object_{};
+  std::vector<const ScopeTrackedObject*> tracked_object_stack_;
   bool deferred_deleting_{};
   MonotonicTime approximate_monotonic_time_;
   WatchdogRegistrationPtr watchdog_registration_;

diff --git a/test/common/event/dispatcher_impl_test.cc b/test/common/event/dispatcher_impl_test.cc
@@ -535,7 +535,9 @@ TEST_F(DispatcherImplTest, IsThreadSafe) {
 
 class TestFatalAction : public Server::Configuration::FatalAction {
 public:
-  void run(const ScopeTrackedObject* /*current_object*/) override { ++times_ran_; }
+  void run(const std::vector<const ScopeTrackedObject*>& /*tracked_objects*/) override {
+    ++times_ran_;
+  }
   bool isAsyncSignalSafe() const override { return true; }
   int getNumTimesRan() { return times_ran_; }
 

diff --git a/test/common/event/scaled_range_timer_manager_impl_test.cc b/test/common/event/scaled_range_timer_manager_impl_test.cc
@@ -1,5 +1,6 @@
 #include <chrono>
 
+#include "envoy/common/scope_tracker.h"
 #include "envoy/event/timer.h"
 
 #include "common/event/dispatcher_impl.h"
@@ -24,9 +25,14 @@ class ScopeTrackingDispatcher : public WrappedDispatcher {
   ScopeTrackingDispatcher(DispatcherPtr dispatcher)
       : WrappedDispatcher(*dispatcher), dispatcher_(std::move(dispatcher)) {}
 
-  const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) override {
+  void appendTrackedObject(const ScopeTrackedObject* object) override {
     scope_ = object;
-    return impl_.setTrackedObject(object);
+    return impl_.appendTrackedObject(object);
+  }
+
+  void popTrackedObject(const ScopeTrackedObject* expected_object) override {
+    scope_ = nullptr;
+    return impl_.popTrackedObject(expected_object);
   }
 
   const ScopeTrackedObject* scope_{nullptr};

diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc
@@ -2654,7 +2654,8 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutCallbackDisarmsAndReturns408
     EXPECT_CALL(response_encoder_, encodeData(_, true)).WillOnce(AddBufferToString(&response_body));
 
     conn_manager_->newStream(response_encoder_);
-    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)).Times(2);
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, appendTrackedObject(_));
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, popTrackedObject(_));
     request_timer->invokeCallback();
     return Http::okStatus();
   }));
@@ -2884,7 +2885,8 @@ TEST_F(HttpConnectionManagerImplTest, RequestHeaderTimeoutCallbackDisarmsAndRetu
     EXPECT_CALL(*request_header_timer, enableTimer(request_headers_timeout_, _));
 
     conn_manager_->newStream(response_encoder_);
-    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)).Times(2);
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, appendTrackedObject(_));
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, popTrackedObject(_));
     return Http::okStatus();
   }));
 

diff --git a/test/common/http/conn_manager_impl_test_2.cc b/test/common/http/conn_manager_impl_test_2.cc
@@ -2348,19 +2348,19 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) {
   {
     RequestHeaderMapPtr headers{
         new TestRequestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "POST"}}};
-    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_))
-        .Times(2)
-        .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* {
+
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, appendTrackedObject(_))
+        .Times(1)
+        .WillOnce(Invoke([](const ScopeTrackedObject* object) -> void {
           ASSERT(object != nullptr); // On the first call, this should be the active stream.
           std::stringstream out;
           object->dumpState(out);
           std::string state = out.str();
           EXPECT_THAT(state,
                       testing::HasSubstr("filter_manager_callbacks_.requestHeaders():   null"));
           EXPECT_THAT(state, testing::HasSubstr("protocol_: 1"));
-          return nullptr;
-        }))
-        .WillRepeatedly(Return(nullptr));
+        }));
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, popTrackedObject(_));
     EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false))
         .WillOnce(Invoke([](HeaderMap&, bool) -> FilterHeadersStatus {
           return FilterHeadersStatus::StopIteration;
@@ -2371,19 +2371,18 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) {
   // Send trailers to that stream, and verify by this point headers are in logged state.
   {
     RequestTrailerMapPtr trailers{new TestRequestTrailerMapImpl{{"foo", "bar"}}};
-    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_))
-        .Times(2)
-        .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* {
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, appendTrackedObject(_))
+        .Times(1)
+        .WillOnce(Invoke([](const ScopeTrackedObject* object) -> void {
           ASSERT(object != nullptr); // On the first call, this should be the active stream.
           std::stringstream out;
           object->dumpState(out);
           std::string state = out.str();
           EXPECT_THAT(state, testing::HasSubstr("filter_manager_callbacks_.requestHeaders(): \n"));
           EXPECT_THAT(state, testing::HasSubstr("':authority', 'host'\n"));
           EXPECT_THAT(state, testing::HasSubstr("protocol_: 1"));
-          return nullptr;
-        }))
-        .WillRepeatedly(Return(nullptr));
+        }));
+    EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, popTrackedObject(_));
     EXPECT_CALL(*decoder_filters_[0], decodeComplete());
     EXPECT_CALL(*decoder_filters_[0], decodeTrailers(_))
         .WillOnce(Return(FilterTrailersStatus::StopIteration));

diff --git a/test/common/http/filter_manager_test.cc b/test/common/http/filter_manager_test.cc
@@ -198,7 +198,8 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionDecodingHeaders) {
 TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) {
   initialize();
 
-  EXPECT_CALL(dispatcher_, setTrackedObject(_)).Times(2);
+  EXPECT_CALL(dispatcher_, appendTrackedObject(_));
+  EXPECT_CALL(dispatcher_, popTrackedObject(_));
 
   // This stream filter will skip further callbacks once it sees both the request and response
   // header. As such, it should see the decoding callbacks but none of the encoding callbacks.
@@ -251,4 +252,4 @@ TEST_F(FilterManagerTest, MatchTreeSkipActionRequestAndResponseHeaders) {
 }
 } // namespace
 } // namespace Http
-} // namespace Envoy
+} // namespace Envoy
diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc
@@ -115,8 +115,9 @@ class RouterTestBase : public testing::Test {
     // Make the "system time" non-zero, because 0 is considered invalid by DateUtil.
     test_time_.setMonotonicTime(std::chrono::milliseconds(50));
 
-    // Allow any number of setTrackedObject calls for the dispatcher strict mock.
-    EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(AnyNumber());
+    // Allow any number of (append|pop)TrackedObject calls for the dispatcher strict mock.
+    EXPECT_CALL(callbacks_.dispatcher_, appendTrackedObject(_)).Times(AnyNumber());
+    EXPECT_CALL(callbacks_.dispatcher_, popTrackedObject(_)).Times(AnyNumber());
   }
 
   void expectResponseTimerCreate() {
@@ -289,15 +290,16 @@ class RouterTestBase : public testing::Test {
       EXPECT_CALL(callbacks_.dispatcher_, createTimer_(_));
     }
     EXPECT_CALL(cm_.thread_local_cluster_.conn_pool_, newStream(_, _))
-        .WillOnce(Invoke(
-            [&](Http::ResponseDecoder& decoder,
-                Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* {
-              response_decoder_ = &decoder;
-              EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(testing::AtLeast(2));
-              callbacks.onPoolReady(original_encoder_, cm_.thread_local_cluster_.conn_pool_.host_,
-                                    upstream_stream_info_, Http::Protocol::Http10);
-              return nullptr;
-            }));
+        .WillOnce(Invoke([&](Http::ResponseDecoder& decoder,
+                             Http::ConnectionPool::Callbacks& callbacks)
+                             -> Http::ConnectionPool::Cancellable* {
+          response_decoder_ = &decoder;
+          EXPECT_CALL(callbacks_.dispatcher_, appendTrackedObject(_)).Times(testing::AtLeast(1));
+          EXPECT_CALL(callbacks_.dispatcher_, popTrackedObject(_)).Times(testing::AtLeast(1));
+          callbacks.onPoolReady(original_encoder_, cm_.thread_local_cluster_.conn_pool_.host_,
+                                upstream_stream_info_, Http::Protocol::Http10);
+          return nullptr;
+        }));
     HttpTestUtility::addDefaultHeaders(default_request_headers_);
     router_.decodeHeaders(default_request_headers_, end_stream);
   }
@@ -2189,15 +2191,17 @@ TEST_F(RouterTest, GrpcOk) {
   EXPECT_EQ(1U,
             callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value());
 
-  EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(2);
+  EXPECT_CALL(callbacks_.dispatcher_, appendTrackedObject(_));
+  EXPECT_CALL(callbacks_.dispatcher_, popTrackedObject(_));
   Http::ResponseHeaderMapPtr response_headers(
       new Http::TestResponseHeaderMapImpl{{":status", "200"}});
   EXPECT_CALL(cm_.thread_local_cluster_.conn_pool_.host_->outlier_detector_,
               putHttpResponseCode(200));
   response_decoder->decodeHeaders(std::move(response_headers), false);
   EXPECT_TRUE(verifyHostUpstreamStats(0, 0));
 
-  EXPECT_CALL(callbacks_.dispatcher_, setTrackedObject(_)).Times(2);
+  EXPECT_CALL(callbacks_.dispatcher_, appendTrackedObject(_));
+  EXPECT_CALL(callbacks_.dispatcher_, popTrackedObject(_));
   Http::ResponseTrailerMapPtr response_trailers(
       new Http::TestResponseTrailerMapImpl{{"grpc-status", "0"}});
   response_decoder->decodeTrailers(std::move(response_trailers));