From d260dcdcc8f66447a98becaf8a033c506585674e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jun 2019 14:33:53 -0400 Subject: [PATCH 01/11] Basic logging functions Signed-off-by: Alyssa Wilk --- include/envoy/http/BUILD | 1 + include/envoy/http/header_map.h | 23 +++++++--- source/common/common/BUILD | 7 +++ source/common/common/log_state_utils.h | 47 ++++++++++++++++++++ source/common/http/BUILD | 1 + source/common/http/conn_manager_impl.cc | 1 + source/common/http/conn_manager_impl.h | 19 ++++++++ source/common/stream_info/BUILD | 1 + source/common/stream_info/stream_info_impl.h | 13 ++++++ test/common/http/conn_manager_impl_test.cc | 1 + 10 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 source/common/common/log_state_utils.h diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index 4f0355015ce3..4d3f288c8380 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -76,6 +76,7 @@ envoy_cc_library( deps = [ "//source/common/common:assert_lib", "//source/common/common:hash_lib", + "//source/common/common:log_state_utils", ], ) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index a9c0ea95058f..c208c08d78a1 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -13,6 +13,7 @@ #include "common/common/assert.h" #include "common/common/hash.h" +#include "common/common/log_state_utils.h" #include "common/common/macros.h" #include "absl/strings/string_view.h" @@ -525,19 +526,27 @@ class HeaderMap { */ virtual bool empty() const PURE; + void logState(std::ostream& os, int indent_level = 0) const { + typedef std::pair IterateData; + const char* spaces = spacesForLevel(indent_level); + IterateData iterate_data = std::make_pair(&os, spaces); + iterate( + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + IterateData* data = static_cast(context); + *data->first << data->second << "'" << header.key().getStringView() << "', '" + << header.value().getStringView() << "'\n"; + return HeaderMap::Iterate::Continue; + }, + &iterate_data); + } + /** * Allow easy pretty-printing of the key/value pairs in HeaderMap * @param os supplies the ostream to print to. * @param headers the headers to print. */ friend std::ostream& operator<<(std::ostream& os, const HeaderMap& headers) { - headers.iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - *static_cast(context) << "'" << header.key().getStringView() << "', '" - << header.value().getStringView() << "'\n"; - return HeaderMap::Iterate::Continue; - }, - &os); + headers.logState(os); return os; } }; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index bacfa4d4b06a..ae889aaee5fb 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -109,6 +109,12 @@ envoy_cc_library( deps = [":assert_lib"], ) +# Contains macros and helpers for logState utilities +envoy_cc_library( + name = "log_state_utils", + hdrs = ["log_state_utils.h"], +) + # Contains minimal code for logging to stderr. envoy_cc_library( name = "minimal_logger_lib", @@ -140,6 +146,7 @@ envoy_cc_library( srcs = ["logger_delegates.cc"], hdrs = ["logger_delegates.h"], deps = [ + ":log_state_utils", ":macros", ":minimal_logger_lib", "//include/envoy/access_log:access_log_interface", diff --git a/source/common/common/log_state_utils.h b/source/common/common/log_state_utils.h new file mode 100644 index 000000000000..5558fc3d2376 --- /dev/null +++ b/source/common/common/log_state_utils.h @@ -0,0 +1,47 @@ +#pragma once + +namespace Envoy { + +#define LOG_MEMBER(member) ", " #member ": " << (member) + +#define LOG_OPTIONAL_MEMBER(member) \ + ", " #member ": " << (member.has_value() ? absl::StrCat(member.value()) : "null") + +// Macro assumes local member variables +// os (ostream) +// indent_level (int) +#define LOG_DETAILS(member) \ + do { \ + os << spaces << #member ": "; \ + if (member != nullptr) { \ + os << "\n"; \ + (member)->logState(os, indent_level + 1); \ + } else { \ + os << spaces << "null\n"; \ + } \ + } while (false) + +// Return the const char* equivalent of string(level*2, ' '), without dealing +// with string creation overhead. Cap arbitrarily at 6 as we're (hopefully) +// not going to have nested objects deeper than that. +inline const char* spacesForLevel(int level) { + switch (level) { + case 0: + return ""; + case 1: + return " "; + case 2: + return " "; + case 3: + return " "; + case 4: + return " "; + case 5: + return " "; + default: + return " "; + } + return ""; +} + +} // namespace Envoy diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 63ee261385d7..2b1279597bde 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -174,6 +174,7 @@ envoy_cc_library( "//source/common/common:empty_string", "//source/common/common:enum_to_int", "//source/common/common:linked_object", + "//source/common/common:log_state_utils", "//source/common/common:utility_lib", "//source/common/http/http1:codec_lib", "//source/common/http/http2:codec_lib", diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 0c5a221b7bf9..7878074c5aa5 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -1535,6 +1535,7 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt void ConnectionManagerImpl::ActiveStream::maybeEndEncode(bool end_stream) { if (end_stream) { + ENVOY_STREAM_LOG(error, "Wrapping up sesssion \n{}", *this, *this); ASSERT(!state_.codec_saw_local_complete_); state_.codec_saw_local_complete_ = true; stream_info_.onLastDownstreamTxByteSent(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 66f819d3abc3..ee1030bd31aa 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -28,6 +28,7 @@ #include "common/buffer/watermark_buffer.h" #include "common/common/linked_object.h" +#include "common/common/log_state_utils.h" #include "common/grpc/common.h" #include "common/http/conn_manager_config.h" #include "common/http/user_agent.h" @@ -494,6 +495,24 @@ class ConnectionManagerImpl : Logger::Loggable, bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); } + void logState(std::ostream& os, int indent_level = 0) const { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "ActiveStream " << this << LOG_MEMBER(stream_id_) + << LOG_MEMBER(has_continue_headers_) << LOG_MEMBER(is_head_request_) + << LOG_MEMBER(decoding_headers_only_) << LOG_MEMBER(encoding_headers_only_) << "\n"; + + LOG_DETAILS(request_headers_); + LOG_DETAILS(request_trailers_); + LOG_DETAILS(response_headers_); + LOG_DETAILS(response_trailers_); + LOG_DETAILS(&stream_info_); + } + + friend std::ostream& operator<<(std::ostream& os, const ActiveStream& s) { + s.logState(os); + return os; + } + ConnectionManagerImpl& connection_manager_; Router::ConfigConstSharedPtr snapped_route_config_; Router::ScopedConfigConstSharedPtr snapped_scoped_route_config_; diff --git a/source/common/stream_info/BUILD b/source/common/stream_info/BUILD index 639805f711f6..006265a9ac5a 100644 --- a/source/common/stream_info/BUILD +++ b/source/common/stream_info/BUILD @@ -15,6 +15,7 @@ envoy_cc_library( ":filter_state_lib", "//include/envoy/stream_info:stream_info_interface", "//source/common/common:assert_lib", + "//source/common/common:log_state_utils", ], ) diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index d435a060e278..ffd0dc6141ba 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -7,6 +7,7 @@ #include "envoy/stream_info/stream_info.h" #include "common/common/assert.h" +#include "common/common/log_state_utils.h" #include "common/stream_info/filter_state_impl.h" namespace Envoy { @@ -208,6 +209,18 @@ struct StreamInfoImpl : public StreamInfo { return upstream_transport_failure_reason_; } + void logState(std::ostream& os, int indent_level = 0) const { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "StreamInfoImpl " << this + << LOG_OPTIONAL_MEMBER(protocol_) + << LOG_OPTIONAL_MEMBER(response_code_) + << LOG_OPTIONAL_MEMBER(response_code_details_) + << LOG_MEMBER(health_check_request_) + << LOG_MEMBER(route_name_) + << "\n"; + } + + TimeSource& time_source_; const SystemTime start_time_; const MonotonicTime start_time_monotonic_; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 4a407492b78f..66e2425b0b09 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -4294,5 +4294,6 @@ TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenDraining) { Buffer::OwnedImpl fake_input; conn_manager_->onData(fake_input, false); } + } // namespace Http } // namespace Envoy From ac937038e809bb3e693881ebe0b797730b23b5cd Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 20 Jun 2019 17:01:21 -0400 Subject: [PATCH 02/11] http: active session tracking, and crash logging on the request path Signed-off-by: Alyssa Wilk --- include/envoy/common/BUILD | 5 ++++ include/envoy/common/scope_tracker.h | 27 +++++++++++++++++ include/envoy/event/BUILD | 1 + include/envoy/event/dispatcher.h | 10 +++++++ source/common/common/BUILD | 8 +++++ source/common/common/log_state_utils.h | 2 ++ source/common/common/scope_tracker.h | 22 ++++++++++++++ source/common/event/BUILD | 8 ++++- source/common/event/dispatcher_impl.cc | 12 ++++++-- source/common/event/dispatcher_impl.h | 24 ++++++++++++++- source/common/http/BUILD | 2 ++ source/common/http/conn_manager_impl.cc | 8 ++++- source/common/http/conn_manager_impl.h | 31 +++++++++++--------- source/common/stream_info/stream_info_impl.h | 11 ++----- source/exe/BUILD | 1 + source/exe/signal_action.cc | 29 ++++++++++++++++++ source/exe/signal_action.h | 24 +++++++++++++++ test/common/http/conn_manager_impl_test.cc | 1 - test/mocks/event/mocks.h | 1 + 19 files changed, 199 insertions(+), 28 deletions(-) create mode 100644 include/envoy/common/scope_tracker.h create mode 100644 source/common/common/scope_tracker.h diff --git a/include/envoy/common/BUILD b/include/envoy/common/BUILD index 1929682fb13c..4e04008c25d0 100644 --- a/include/envoy/common/BUILD +++ b/include/envoy/common/BUILD @@ -51,3 +51,8 @@ envoy_cc_library( name = "backoff_strategy_interface", hdrs = ["backoff_strategy.h"], ) + +envoy_cc_library( + name = "scope_tracker_interface", + hdrs = ["scope_tracker.h"], +) diff --git a/include/envoy/common/scope_tracker.h b/include/envoy/common/scope_tracker.h new file mode 100644 index 000000000000..b813eccbb09b --- /dev/null +++ b/include/envoy/common/scope_tracker.h @@ -0,0 +1,27 @@ +#pragma once + +#include + +#include "envoy/common/pure.h" + +namespace Envoy { + +/* + * A class for tracking the scope of work. + * Currently this is only used for best-effort tracking the any L7 stream doing + * work if a crash occurs. + */ +class ScopeTrackedObject { +public: + virtual ~ScopeTrackedObject() {} + + /** + * Dump debug state of the object in question to the provided ostream + * + * @param os the ostream to output to. + * @param indent_level how far to indent, for pretty-printed classes and subclasses. + */ + virtual void logState(std::ostream& os, int indent_level = 0) const PURE; +}; + +} // namespace Envoy diff --git a/include/envoy/event/BUILD b/include/envoy/event/BUILD index 962e290e3808..05ea911bf8cc 100644 --- a/include/envoy/event/BUILD +++ b/include/envoy/event/BUILD @@ -20,6 +20,7 @@ envoy_cc_library( ":deferred_deletable", ":file_event_interface", ":signal_interface", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/common:time_interface", "//include/envoy/event:timer_interface", "//include/envoy/filesystem:watcher_interface", diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 50a6ddbadb86..24e3516b5b01 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -6,6 +6,7 @@ #include #include +#include "envoy/common/scope_tracker.h" #include "envoy/common/time.h" #include "envoy/event/file_event.h" #include "envoy/event/signal.h" @@ -199,6 +200,15 @@ class Dispatcher { * @return the watermark buffer factory for this dispatcher. */ virtual Buffer::WatermarkFactory& getWatermarkFactory() 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. + * + * @return The previously tracked object or nullptr if there was none. + */ + virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; }; using DispatcherPtr = std::unique_ptr; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index ae889aaee5fb..a88201ad891b 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -184,6 +184,14 @@ envoy_cc_library( hdrs = ["phantom.h"], ) +envoy_cc_library( + name = "scope_tracker", + hdrs = ["scope_tracker.h"], + deps = [ + "//include/envoy/common:scope_tracker_interface", + ], +) + envoy_cc_library( name = "stl_helpers", hdrs = ["stl_helpers.h"], diff --git a/source/common/common/log_state_utils.h b/source/common/common/log_state_utils.h index 5558fc3d2376..28b4526c3088 100644 --- a/source/common/common/log_state_utils.h +++ b/source/common/common/log_state_utils.h @@ -1,5 +1,7 @@ #pragma once +#include + namespace Envoy { #define LOG_MEMBER(member) ", " #member ": " << (member) diff --git a/source/common/common/scope_tracker.h b/source/common/common/scope_tracker.h new file mode 100644 index 000000000000..446f0a1a8487 --- /dev/null +++ b/source/common/common/scope_tracker.h @@ -0,0 +1,22 @@ +#pragma once + +#include "envoy/common/scope_tracker.h" +#include "envoy/event/dispatcher.h" + +namespace Envoy { + +class ScopeTrackerImpl { +public: + ScopeTrackerImpl(const ScopeTrackedObject* object, Event::Dispatcher& dispatcher) + : dispatcher_(dispatcher) { + latched_object_ = dispatcher_.setTrackedObject(object); + } + + ~ScopeTrackerImpl() { dispatcher_.setTrackedObject(latched_object_); } + +private: + const ScopeTrackedObject* latched_object_; + Event::Dispatcher& dispatcher_; +}; + +} // namespace Envoy diff --git a/source/common/event/BUILD b/source/common/event/BUILD index 478fc28eb4c8..51cc6c213f24 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -22,6 +22,7 @@ envoy_cc_library( ":dispatcher_includes", ":libevent_scheduler_lib", ":real_time_system_lib", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/common:time_interface", "//include/envoy/event:signal_interface", "//include/envoy/network:listen_socket_interface", @@ -74,7 +75,12 @@ envoy_cc_library( "//include/envoy/network:connection_handler_interface", "//source/common/common:minimal_logger_lib", "//source/common/common:thread_lib", - ], + ] + select({ + "//bazel:disable_signal_trace": [], + "//conditions:default": [ + "//source/exe:sigaction_lib", + ], + }), ) envoy_cc_library( diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 4ffcf6181a82..be041999956e 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -37,9 +37,17 @@ DispatcherImpl::DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& scheduler_(time_system.createScheduler(base_scheduler_)), deferred_delete_timer_(createTimer([this]() -> void { clearDeferredDeleteList(); })), post_timer_(createTimer([this]() -> void { runPostCallbacks(); })), - current_to_delete_(&to_delete_1_) {} + current_to_delete_(&to_delete_1_) { +#ifdef ENVOY_HANDLE_SIGNALS + SignalAction::registerCrashHandler(*this); +#endif +} -DispatcherImpl::~DispatcherImpl() {} +DispatcherImpl::~DispatcherImpl() { +#ifdef ENVOY_HANDLE_SIGNALS + SignalAction::removeCrashHandler(*this); +#endif +} void DispatcherImpl::initializeStats(Stats::Scope& scope, const std::string& prefix) { // This needs to be run in the dispatcher's thread, so that we have a thread id to log. diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index b712f22d879e..812fa6fef64d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -7,6 +7,7 @@ #include #include "envoy/api/api.h" +#include "envoy/common/scope_tracker.h" #include "envoy/common/time.h" #include "envoy/event/deferred_deletable.h" #include "envoy/event/dispatcher.h" @@ -18,13 +19,19 @@ #include "common/event/libevent.h" #include "common/event/libevent_scheduler.h" +#ifdef ENVOY_HANDLE_SIGNALS +#include "exe/signal_action.h" +#endif + namespace Envoy { namespace Event { /** * libevent implementation of Event::Dispatcher. */ -class DispatcherImpl : Logger::Loggable, public Dispatcher { +class DispatcherImpl : Logger::Loggable, + public Dispatcher, + public CrashHandlerInterface { public: DispatcherImpl(Api::Api& api, Event::TimeSystem& time_system); DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& api, @@ -65,6 +72,20 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { void post(std::function 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; + } + + // CrashHandlerInterface + void crashHandler() const override { + if (isThreadSafe()) { + if (current_object_) { + current_object_->logState(std::cerr); + } + } + } private: void runPostCallbacks(); @@ -88,6 +109,7 @@ class DispatcherImpl : Logger::Loggable, public Dispatcher { std::vector* current_to_delete_; Thread::MutexBasicLockable post_lock_; std::list> post_callbacks_ GUARDED_BY(post_lock_); + const ScopeTrackedObject* current_object_{}; bool deferred_deleting_{}; }; diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 2b1279597bde..16be9f55297f 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -148,6 +148,7 @@ envoy_cc_library( ":utility_lib", "//include/envoy/access_log:access_log_interface", "//include/envoy/buffer:buffer_interface", + "//include/envoy/common:scope_tracker_interface", "//include/envoy/common:time_interface", "//include/envoy/event:deferred_deletable", "//include/envoy/event:dispatcher_interface", @@ -175,6 +176,7 @@ envoy_cc_library( "//source/common/common:enum_to_int", "//source/common/common:linked_object", "//source/common/common:log_state_utils", + "//source/common/common:scope_tracker", "//source/common/common:utility_lib", "//source/common/http/http1:codec_lib", "//source/common/http/http2:codec_lib", diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 7878074c5aa5..32de255fbc10 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -21,6 +21,7 @@ #include "common/common/empty_string.h" #include "common/common/enum_to_int.h" #include "common/common/fmt.h" +#include "common/common/scope_tracker.h" #include "common/common/utility.h" #include "common/http/codes.h" #include "common/http/conn_manager_utility.h" @@ -416,6 +417,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()), upstream_options_(std::make_shared()) { + ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + connection_manager_.stats_.named_.downstream_rq_total_.inc(); connection_manager_.stats_.named_.downstream_rq_active_.inc(); if (connection_manager_.codec_->protocol() == Protocol::Http2) { @@ -585,6 +588,7 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() { // TODO(alyssawilk) all the calls here should be audited for order priority, // e.g. many early returns do not currently handle connection: close properly. void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { + ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); request_headers_ = std::move(headers); if (Http::Headers::get().MethodValues.Head == request_headers_->Method()->value().getStringView()) { @@ -881,6 +885,7 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte } void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, bool end_stream) { + ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); maybeEndDecode(end_stream); stream_info_.addBytesReceived(data.length()); @@ -890,6 +895,7 @@ void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, boo void ConnectionManagerImpl::ActiveStream::decodeData( ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream, FilterIterationStartState filter_iteration_start_state) { + ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); resetIdleTimer(); // If we previously decided to decode only the headers, do nothing here. @@ -1029,6 +1035,7 @@ void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilt } void ConnectionManagerImpl::ActiveStream::decodeTrailers(HeaderMapPtr&& trailers) { + ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); resetIdleTimer(); maybeEndDecode(true); request_trailers_ = std::move(trailers); @@ -1535,7 +1542,6 @@ void ConnectionManagerImpl::ActiveStream::encodeTrailers(ActiveStreamEncoderFilt void ConnectionManagerImpl::ActiveStream::maybeEndEncode(bool end_stream) { if (end_stream) { - ENVOY_STREAM_LOG(error, "Wrapping up sesssion \n{}", *this, *this); ASSERT(!state_.codec_saw_local_complete_); state_.codec_saw_local_complete_ = true; stream_info_.onLastDownstreamTxByteSent(); diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index ee1030bd31aa..bfffa74b8ad0 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -9,6 +9,7 @@ #include #include "envoy/access_log/access_log.h" +#include "envoy/common/scope_tracker.h" #include "envoy/event/deferred_deletable.h" #include "envoy/http/codec.h" #include "envoy/http/codes.h" @@ -335,7 +336,8 @@ class ConnectionManagerImpl : Logger::Loggable, public StreamCallbacks, public StreamDecoder, public FilterChainFactoryCallbacks, - public Tracing::Config { + public Tracing::Config, + public ScopeTrackedObject { ActiveStream(ConnectionManagerImpl& connection_manager); ~ActiveStream(); @@ -418,6 +420,20 @@ class ConnectionManagerImpl : Logger::Loggable, const std::vector& requestHeadersForTags() const override; bool verbose() const override; + // ScopeTrackedObject + void logState(std::ostream& os, int indent_level = 0) const override { + const char* spaces = spacesForLevel(indent_level); + os << spaces << "ActiveStream " << this << LOG_MEMBER(stream_id_) + << LOG_MEMBER(has_continue_headers_) << LOG_MEMBER(is_head_request_) + << LOG_MEMBER(decoding_headers_only_) << LOG_MEMBER(encoding_headers_only_) << "\n"; + + LOG_DETAILS(request_headers_); + LOG_DETAILS(request_trailers_); + LOG_DETAILS(response_headers_); + LOG_DETAILS(response_trailers_); + LOG_DETAILS(&stream_info_); + } + void traceRequest(); void refreshCachedRoute(); @@ -495,19 +511,6 @@ class ConnectionManagerImpl : Logger::Loggable, bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); } - void logState(std::ostream& os, int indent_level = 0) const { - const char* spaces = spacesForLevel(indent_level); - os << spaces << "ActiveStream " << this << LOG_MEMBER(stream_id_) - << LOG_MEMBER(has_continue_headers_) << LOG_MEMBER(is_head_request_) - << LOG_MEMBER(decoding_headers_only_) << LOG_MEMBER(encoding_headers_only_) << "\n"; - - LOG_DETAILS(request_headers_); - LOG_DETAILS(request_trailers_); - LOG_DETAILS(response_headers_); - LOG_DETAILS(response_trailers_); - LOG_DETAILS(&stream_info_); - } - friend std::ostream& operator<<(std::ostream& os, const ActiveStream& s) { s.logState(os); return os; diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index ffd0dc6141ba..af4ee647a2ec 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -211,16 +211,11 @@ struct StreamInfoImpl : public StreamInfo { void logState(std::ostream& os, int indent_level = 0) const { const char* spaces = spacesForLevel(indent_level); - os << spaces << "StreamInfoImpl " << this - << LOG_OPTIONAL_MEMBER(protocol_) - << LOG_OPTIONAL_MEMBER(response_code_) - << LOG_OPTIONAL_MEMBER(response_code_details_) - << LOG_MEMBER(health_check_request_) - << LOG_MEMBER(route_name_) - << "\n"; + os << spaces << "StreamInfoImpl " << this << LOG_OPTIONAL_MEMBER(protocol_) + << LOG_OPTIONAL_MEMBER(response_code_) << LOG_OPTIONAL_MEMBER(response_code_details_) + << LOG_MEMBER(health_check_request_) << LOG_MEMBER(route_name_) << "\n"; } - TimeSource& time_source_; const SystemTime start_time_; const MonotonicTime start_time_monotonic_; diff --git a/source/exe/BUILD b/source/exe/BUILD index 67cb796c249a..81eb7fec23b3 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -124,6 +124,7 @@ envoy_cc_library( deps = [ "//source/common/common:assert_lib", "//source/common/common:non_copyable", + "//source/common/singleton:threadsafe_singleton", "//source/server:backtrace_lib", ], ) diff --git a/source/exe/signal_action.cc b/source/exe/signal_action.cc index 00cfaf5d2c40..d527c895bb35 100644 --- a/source/exe/signal_action.cc +++ b/source/exe/signal_action.cc @@ -7,6 +7,28 @@ #include "common/common/assert.h" namespace Envoy { + +ABSL_CONST_INIT static absl::Mutex failure_mutex(absl::kConstInit); +using FailureFunctionList = std::list; +ABSL_CONST_INIT std::atomic crash_handlers{nullptr}; + +void SignalAction::registerCrashHandler(const CrashHandlerInterface& handler) { + absl::MutexLock l(&failure_mutex); + FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + if (list == nullptr) { + list = new FailureFunctionList; + } + list->push_back(&handler); + crash_handlers.store(list, std::memory_order_release); +} + +void SignalAction::removeCrashHandler(const CrashHandlerInterface& handler) { + absl::MutexLock l(&failure_mutex); + FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + list->remove(&handler); + crash_handlers.store(list, std::memory_order_release); +} + constexpr int SignalAction::FATAL_SIGS[]; void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) { @@ -20,6 +42,13 @@ void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) { } tracer.logTrace(); + FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + + // Finally after logging the stack trace, call any registered crash handlers. + for (const auto* handler : *list) { + handler->crashHandler(); + } + signal(sig, SIG_DFL); raise(sig); } diff --git a/source/exe/signal_action.h b/source/exe/signal_action.h index 7af81167c5d0..ca00ab2d7ba3 100644 --- a/source/exe/signal_action.h +++ b/source/exe/signal_action.h @@ -4,12 +4,22 @@ #include #include +#include #include "common/common/non_copyable.h" #include "server/backtrace.h" namespace Envoy { + +// A simple class which allows registering functions to be called when Envoy +// receives one of the fatal signals, documented below. +class CrashHandlerInterface { +public: + virtual ~CrashHandlerInterface() {} + virtual void crashHandler() const PURE; +}; + /** * This class installs signal handlers for fatal signal types. * @@ -71,6 +81,18 @@ class SignalAction : NonCopyable { */ static void sigHandler(int sig, siginfo_t* info, void* context); + /** + * Add this handler to the list of functions which will be called if Envoy + * receives a fatal signal. + */ + static void registerCrashHandler(const CrashHandlerInterface& handler); + + /** + * Removes this handler from the list of functions which will be called if Envoy + * receives a fatal signal. + */ + static void removeCrashHandler(const CrashHandlerInterface& handler); + private: /** * Allocate this many bytes on each side of the area used for alt stack. @@ -128,5 +150,7 @@ class SignalAction : NonCopyable { char* altstack_; std::array previous_handlers_; stack_t previous_altstack_; + std::list crash_handlers_; }; + } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 66e2425b0b09..4a407492b78f 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -4294,6 +4294,5 @@ TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenDraining) { Buffer::OwnedImpl fake_input; conn_manager_->onData(fake_input, false); } - } // namespace Http } // namespace Envoy diff --git a/test/mocks/event/mocks.h b/test/mocks/event/mocks.h index 4237a3b57ca0..5cbd2f076f5f 100644 --- a/test/mocks/event/mocks.h +++ b/test/mocks/event/mocks.h @@ -113,6 +113,7 @@ class MockDispatcher : public Dispatcher { MOCK_METHOD2(listenForSignal_, SignalEvent*(int signal_num, SignalCb cb)); MOCK_METHOD1(post, void(std::function callback)); MOCK_METHOD1(run, void(RunType type)); + MOCK_METHOD1(setTrackedObject, const ScopeTrackedObject*(const ScopeTrackedObject* object)); Buffer::WatermarkFactory& getWatermarkFactory() override { return buffer_factory_; } GlobalTimeSystem time_system_; From e0c631d8a1c3a03398b8b3302f95c4bd6a1fffe6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 25 Jun 2019 16:45:48 -0400 Subject: [PATCH 03/11] Fixing build, adding UT Signed-off-by: Alyssa Wilk --- source/common/event/BUILD | 1 + source/common/event/dispatcher_impl.cc | 4 ++ source/common/event/dispatcher_impl.h | 4 +- source/exe/BUILD | 6 +++ source/exe/crash_handler.h | 18 +++++++ source/exe/signal_action.cc | 9 ++-- source/exe/signal_action.h | 10 +--- test/common/http/conn_manager_impl_test.cc | 63 ++++++++++++++++++++++ 8 files changed, 100 insertions(+), 15 deletions(-) create mode 100644 source/exe/crash_handler.h diff --git a/source/common/event/BUILD b/source/common/event/BUILD index 51cc6c213f24..d7ed37e65c04 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -75,6 +75,7 @@ envoy_cc_library( "//include/envoy/network:connection_handler_interface", "//source/common/common:minimal_logger_lib", "//source/common/common:thread_lib", + "//source/exe:crash_handler_lib", ] + select({ "//bazel:disable_signal_trace": [], "//conditions:default": [ diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index be041999956e..02e8b9321499 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -25,6 +25,10 @@ #include "event2/event.h" +#ifdef ENVOY_HANDLE_SIGNALS +#include "exe/signal_action.h" +#endif + namespace Envoy { namespace Event { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 812fa6fef64d..ac441a410f14 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -19,9 +19,7 @@ #include "common/event/libevent.h" #include "common/event/libevent_scheduler.h" -#ifdef ENVOY_HANDLE_SIGNALS -#include "exe/signal_action.h" -#endif +#include "exe/crash_handler.h" namespace Envoy { namespace Event { diff --git a/source/exe/BUILD b/source/exe/BUILD index 81eb7fec23b3..5ec455be09aa 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -30,6 +30,11 @@ envoy_cc_binary( deps = ["envoy_main_entry_lib"], ) +envoy_cc_library( + name = "crash_handler_lib", + hdrs = ["crash_handler.h"], +) + envoy_cc_library( name = "envoy_common_lib", deps = [ @@ -122,6 +127,7 @@ envoy_cc_library( hdrs = ["signal_action.h"], tags = ["backtrace"], deps = [ + ":crash_handler_lib", "//source/common/common:assert_lib", "//source/common/common:non_copyable", "//source/common/singleton:threadsafe_singleton", diff --git a/source/exe/crash_handler.h b/source/exe/crash_handler.h new file mode 100644 index 000000000000..7a50fe490710 --- /dev/null +++ b/source/exe/crash_handler.h @@ -0,0 +1,18 @@ +#pragma once + +#include "envoy/common/pure.h" + +namespace Envoy { + +// A simple class which allows registering functions to be called when Envoy +// receives one of the fatal signals, documented below. +// +// This is split out of signal_action.h because it is exempted from various +// builds. +class CrashHandlerInterface { +public: + virtual ~CrashHandlerInterface() {} + virtual void crashHandler() const PURE; +}; + +} // namespace Envoy diff --git a/source/exe/signal_action.cc b/source/exe/signal_action.cc index d527c895bb35..ee915b01d3c4 100644 --- a/source/exe/signal_action.cc +++ b/source/exe/signal_action.cc @@ -43,10 +43,11 @@ void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) { tracer.logTrace(); FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); - - // Finally after logging the stack trace, call any registered crash handlers. - for (const auto* handler : *list) { - handler->crashHandler(); + if (list) { + // Finally after logging the stack trace, call any registered crash handlers. + for (const auto* handler : *list) { + handler->crashHandler(); + } } signal(sig, SIG_DFL); diff --git a/source/exe/signal_action.h b/source/exe/signal_action.h index ca00ab2d7ba3..11e098b51447 100644 --- a/source/exe/signal_action.h +++ b/source/exe/signal_action.h @@ -8,18 +8,12 @@ #include "common/common/non_copyable.h" +#include "exe/crash_handler.h" + #include "server/backtrace.h" namespace Envoy { -// A simple class which allows registering functions to be called when Envoy -// receives one of the fatal signals, documented below. -class CrashHandlerInterface { -public: - virtual ~CrashHandlerInterface() {} - virtual void crashHandler() const PURE; -}; - /** * This class installs signal handlers for fatal signal types. * diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 4a407492b78f..036a3ed64ef4 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -51,6 +51,7 @@ using testing::AnyNumber; using testing::AtLeast; using testing::DoAll; using testing::Eq; +using testing::HasSubstr; using testing::InSequence; using testing::Invoke; using testing::InvokeWithoutArgs; @@ -4294,5 +4295,67 @@ TEST_F(HttpConnectionManagerImplTest, DisableKeepAliveWhenDraining) { Buffer::OwnedImpl fake_input; conn_manager_->onData(fake_input, false); } + +TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) { + setup(false, ""); + + // Set up the codec. + EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance& data) -> void { + data.drain(4); + })); + Buffer::OwnedImpl fake_input("1234"); + conn_manager_->onData(fake_input, false); + + setupFilterChain(1, 1); + + // Create a new stream + StreamDecoder* decoder = &conn_manager_->newStream(response_encoder_); + + // Send headers to that stream, and verify we both set and clear the tracked object. + { + HeaderMapPtr headers{ + new TestHeaderMapImpl{{":authority", "host"}, {":path", "/"}, {":method", "POST"}}}; + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)) + .Times(2) + .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* { + ASSERT(object != nullptr); // On the first call, this should be the active stream. + std::stringstream out; + object->logState(out); + std::string state = out.str(); + EXPECT_THAT(state, testing::HasSubstr("request_headers_: null")); + EXPECT_THAT(state, testing::HasSubstr("protocol_: 1")); + return nullptr; + })) + .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false)) + .WillOnce(Invoke([](HeaderMap&, bool) -> FilterHeadersStatus { + return FilterHeadersStatus::StopIteration; + })); + decoder->decodeHeaders(std::move(headers), false); + } + + // Send trailers to that stream, and verify by this point headers are in logged state. + { + HeaderMapPtr trailers{new TestHeaderMapImpl{{"foo", "bar"}}}; + EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)) + .Times(2) + .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* { + ASSERT(object != nullptr); // On the first call, this should be the active stream. + std::stringstream out; + object->logState(out); + std::string state = out.str(); + EXPECT_THAT(state, testing::HasSubstr("request_headers_: \n")); + EXPECT_THAT(state, testing::HasSubstr("':authority', 'host'\n")); + EXPECT_THAT(state, testing::HasSubstr("protocol_: 1")); + return nullptr; + })) + .WillRepeatedly(Return(nullptr)); + EXPECT_CALL(*decoder_filters_[0], decodeComplete()); + EXPECT_CALL(*decoder_filters_[0], decodeTrailers(_)) + .WillOnce(Return(FilterTrailersStatus::StopIteration)); + decoder->decodeTrailers(std::move(trailers)); + } +} + } // namespace Http } // namespace Envoy From b17c382f2e92f13658533badbf52a3602d3103a6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 26 Jun 2019 09:58:22 -0400 Subject: [PATCH 04/11] tidy + more tests Signed-off-by: Alyssa Wilk --- include/envoy/common/scope_tracker.h | 2 +- include/envoy/http/header_map.h | 4 ++-- source/common/common/BUILD | 1 + source/common/common/log_state_utils.h | 4 ++-- source/common/event/dispatcher_impl.cc | 8 +++++--- source/common/event/dispatcher_impl.h | 1 + source/common/http/conn_manager_impl.h | 2 +- source/exe/crash_handler.h | 2 +- source/exe/signal_action.cc | 6 +++++- source/exe/signal_action.h | 5 ++--- test/exe/signals_test.cc | 28 ++++++++++++++++++++++++++ 11 files changed, 49 insertions(+), 14 deletions(-) diff --git a/include/envoy/common/scope_tracker.h b/include/envoy/common/scope_tracker.h index b813eccbb09b..30114951589f 100644 --- a/include/envoy/common/scope_tracker.h +++ b/include/envoy/common/scope_tracker.h @@ -13,7 +13,7 @@ namespace Envoy { */ class ScopeTrackedObject { public: - virtual ~ScopeTrackedObject() {} + virtual ~ScopeTrackedObject() = default; /** * Dump debug state of the object in question to the provided ostream diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index c208c08d78a1..c6488ce59108 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -527,12 +527,12 @@ class HeaderMap { virtual bool empty() const PURE; void logState(std::ostream& os, int indent_level = 0) const { - typedef std::pair IterateData; + using IterateData = std::pair; const char* spaces = spacesForLevel(indent_level); IterateData iterate_data = std::make_pair(&os, spaces); iterate( [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - IterateData* data = static_cast(context); + auto* data = static_cast(context); *data->first << data->second << "'" << header.key().getStringView() << "', '" << header.value().getStringView() << "'\n"; return HeaderMap::Iterate::Continue; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index a88201ad891b..40cee9aef4c9 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -189,6 +189,7 @@ envoy_cc_library( hdrs = ["scope_tracker.h"], deps = [ "//include/envoy/common:scope_tracker_interface", + "//include/envoy/event:dispatcher_interface", ], ) diff --git a/source/common/common/log_state_utils.h b/source/common/common/log_state_utils.h index 28b4526c3088..9f88becb8e55 100644 --- a/source/common/common/log_state_utils.h +++ b/source/common/common/log_state_utils.h @@ -7,7 +7,7 @@ namespace Envoy { #define LOG_MEMBER(member) ", " #member ": " << (member) #define LOG_OPTIONAL_MEMBER(member) \ - ", " #member ": " << (member.has_value() ? absl::StrCat(member.value()) : "null") + ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") // Macro assumes local member variables // os (ostream) @@ -15,7 +15,7 @@ namespace Envoy { #define LOG_DETAILS(member) \ do { \ os << spaces << #member ": "; \ - if (member != nullptr) { \ + if ((member) != nullptr) { \ os << "\n"; \ (member)->logState(os, indent_level + 1); \ } else { \ diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 02e8b9321499..b33842b807b3 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -39,8 +39,8 @@ DispatcherImpl::DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& Event::TimeSystem& time_system) : api_(api), buffer_factory_(std::move(factory)), scheduler_(time_system.createScheduler(base_scheduler_)), - deferred_delete_timer_(createTimer([this]() -> void { clearDeferredDeleteList(); })), - post_timer_(createTimer([this]() -> void { runPostCallbacks(); })), + deferred_delete_timer_(createTimerInternal([this]() -> void { clearDeferredDeleteList(); })), + post_timer_(createTimerInternal([this]() -> void { runPostCallbacks(); })), current_to_delete_(&to_delete_1_) { #ifdef ENVOY_HANDLE_SIGNALS SignalAction::registerCrashHandler(*this); @@ -145,7 +145,9 @@ Network::ListenerPtr DispatcherImpl::createUdpListener(Network::Socket& socket, return Network::ListenerPtr{new Network::UdpListenerImpl(*this, socket, cb, timeSource())}; } -TimerPtr DispatcherImpl::createTimer(TimerCb cb) { +TimerPtr DispatcherImpl::createTimer(TimerCb cb) { return createTimerInternal(cb); } + +TimerPtr DispatcherImpl::createTimerInternal(TimerCb cb) { ASSERT(isThreadSafe()); return scheduler_->createTimer(cb); } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index ac441a410f14..ab278fdd7049 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -86,6 +86,7 @@ class DispatcherImpl : Logger::Loggable, } private: + TimerPtr createTimerInternal(TimerCb cb); void runPostCallbacks(); // Validate that an operation is thread safe, i.e. it's invoked on the same thread that the diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index bfffa74b8ad0..12046c4e6187 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -339,7 +339,7 @@ class ConnectionManagerImpl : Logger::Loggable, public Tracing::Config, public ScopeTrackedObject { ActiveStream(ConnectionManagerImpl& connection_manager); - ~ActiveStream(); + ~ActiveStream() override; // Indicates which filter to start the iteration with. enum class FilterIterationStartState { AlwaysStartFromNext, CanStartFromCurrent }; diff --git a/source/exe/crash_handler.h b/source/exe/crash_handler.h index 7a50fe490710..485b2de8da21 100644 --- a/source/exe/crash_handler.h +++ b/source/exe/crash_handler.h @@ -11,7 +11,7 @@ namespace Envoy { // builds. class CrashHandlerInterface { public: - virtual ~CrashHandlerInterface() {} + virtual ~CrashHandlerInterface() = default; virtual void crashHandler() const PURE; }; diff --git a/source/exe/signal_action.cc b/source/exe/signal_action.cc index ee915b01d3c4..344b11b4d541 100644 --- a/source/exe/signal_action.cc +++ b/source/exe/signal_action.cc @@ -26,7 +26,11 @@ void SignalAction::removeCrashHandler(const CrashHandlerInterface& handler) { absl::MutexLock l(&failure_mutex); FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); list->remove(&handler); - crash_handlers.store(list, std::memory_order_release); + if (list->empty()) { + delete list; + } else { + crash_handlers.store(list, std::memory_order_release); + } } constexpr int SignalAction::FATAL_SIGS[]; diff --git a/source/exe/signal_action.h b/source/exe/signal_action.h index 11e098b51447..15354e166dec 100644 --- a/source/exe/signal_action.h +++ b/source/exe/signal_action.h @@ -54,8 +54,7 @@ class SignalAction : NonCopyable { public: SignalAction() : guard_size_(sysconf(_SC_PAGE_SIZE)), - altstack_size_(std::max(guard_size_ * 4, static_cast(MINSIGSTKSZ))), - altstack_(nullptr) { + altstack_size_(std::max(guard_size_ * 4, static_cast(MINSIGSTKSZ))), altstack_() { mapAndProtectStackMemory(); installSigHandlers(); } @@ -141,7 +140,7 @@ class SignalAction : NonCopyable { * Unmap alternative stack memory. */ void unmapStackMemory(); - char* altstack_; + char* altstack_{}; std::array previous_handlers_; stack_t previous_altstack_; std::list crash_handlers_; diff --git a/test/exe/signals_test.cc b/test/exe/signals_test.cc index eb07fe61370a..4cb533fae2c4 100644 --- a/test/exe/signals_test.cc +++ b/test/exe/signals_test.cc @@ -34,6 +34,33 @@ TEST(SignalsDeathTest, InvalidAddressDeathTest) { "backtrace.*Segmentation fault"); } +class TestCrashHandler : public CrashHandlerInterface { + virtual void crashHandler() const override { std::cerr << "HERE!"; } +}; + +TEST(SignalsDeathTest, RegisteredHandlerTest) { + TestCrashHandler handler; + SignalAction::registerCrashHandler(handler); + SignalAction actions; + EXPECT_DEATH_LOG_TO_STDERR( + []() -> void { + // Oops! + volatile int* nasty_ptr = reinterpret_cast(0x0); + *(nasty_ptr) = 0; + }(), + "backtrace.*Segmentation fault"); + SignalAction::removeCrashHandler(handler); + + /* SignalAction actions; + EXPECT_DEATH_LOG_TO_STDERR( + []() -> void { + // Oops! + volatile int* nasty_ptr = reinterpret_cast(0x0); + *(nasty_ptr) = 0; + }(), + "HERE!");*/ +} + TEST(SignalsDeathTest, BusDeathTest) { SignalAction actions; EXPECT_DEATH_LOG_TO_STDERR( @@ -90,6 +117,7 @@ TEST(SignalsDeathTest, RestoredPreviousHandlerDeathTest) { // Outer SignalAction should be active again: EXPECT_DEATH_LOG_TO_STDERR([]() -> void { abort(); }(), "backtrace.*Abort(ed)?"); } + #endif TEST(SignalsDeathTest, IllegalStackAccessDeathTest) { From 4939235830132875ca5d8d1ce3abd4e81aaf5073 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 26 Jun 2019 17:19:37 -0400 Subject: [PATCH 05/11] reviewer comments Signed-off-by: Alyssa Wilk --- include/envoy/common/scope_tracker.h | 6 +++-- include/envoy/event/dispatcher.h | 2 +- include/envoy/http/BUILD | 1 - include/envoy/http/header_map.h | 25 ++++++++----------- source/common/common/BUILD | 8 +++--- .../{log_state_utils.h => dump_state_utils.h} | 10 +++++--- source/common/event/BUILD | 2 +- source/common/event/dispatcher_impl.cc | 4 +-- source/common/event/dispatcher_impl.h | 10 ++++---- source/common/http/BUILD | 3 ++- source/common/http/conn_manager_impl.h | 24 +++++++++--------- source/common/http/header_map_impl.cc | 15 +++++++++++ source/common/http/header_map_impl.h | 1 + source/common/stream_info/BUILD | 2 +- source/common/stream_info/stream_info_impl.h | 10 ++++---- source/exe/BUILD | 6 ++--- ...{crash_handler.h => fatal_error_handler.h} | 6 ++--- source/exe/signal_action.cc | 20 +++++++-------- source/exe/signal_action.h | 8 +++--- test/common/http/conn_manager_impl_test.cc | 4 +-- test/exe/signals_test.cc | 10 ++++---- 21 files changed, 96 insertions(+), 81 deletions(-) rename source/common/common/{log_state_utils.h => dump_state_utils.h} (83%) rename source/exe/{crash_handler.h => fatal_error_handler.h} (71%) diff --git a/include/envoy/common/scope_tracker.h b/include/envoy/common/scope_tracker.h index 30114951589f..626e6a2bac6b 100644 --- a/include/envoy/common/scope_tracker.h +++ b/include/envoy/common/scope_tracker.h @@ -9,7 +9,7 @@ namespace Envoy { /* * A class for tracking the scope of work. * Currently this is only used for best-effort tracking the any L7 stream doing - * work if a crash occurs. + * work if a fatal error occurs. */ class ScopeTrackedObject { public: @@ -18,10 +18,12 @@ class ScopeTrackedObject { /** * Dump debug state of the object in question to the provided ostream * + * This is called on Envoy fatal errors, so should do minimal memory allocation. + * * @param os the ostream to output to. * @param indent_level how far to indent, for pretty-printed classes and subclasses. */ - virtual void logState(std::ostream& os, int indent_level = 0) const PURE; + virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE; }; } // namespace Envoy diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 24e3516b5b01..4b9ffbf4e0ee 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -204,7 +204,7 @@ class Dispatcher { /** * 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. + * work. Calling setTrackedObject(nullptr) results in no object being tracked. * * @return The previously tracked object or nullptr if there was none. */ diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD index 4d3f288c8380..4f0355015ce3 100644 --- a/include/envoy/http/BUILD +++ b/include/envoy/http/BUILD @@ -76,7 +76,6 @@ envoy_cc_library( deps = [ "//source/common/common:assert_lib", "//source/common/common:hash_lib", - "//source/common/common:log_state_utils", ], ) diff --git a/include/envoy/http/header_map.h b/include/envoy/http/header_map.h index c6488ce59108..01828dc2a28d 100644 --- a/include/envoy/http/header_map.h +++ b/include/envoy/http/header_map.h @@ -13,7 +13,6 @@ #include "common/common/assert.h" #include "common/common/hash.h" -#include "common/common/log_state_utils.h" #include "common/common/macros.h" #include "absl/strings/string_view.h" @@ -526,19 +525,15 @@ class HeaderMap { */ virtual bool empty() const PURE; - void logState(std::ostream& os, int indent_level = 0) const { - using IterateData = std::pair; - const char* spaces = spacesForLevel(indent_level); - IterateData iterate_data = std::make_pair(&os, spaces); - iterate( - [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { - auto* data = static_cast(context); - *data->first << data->second << "'" << header.key().getStringView() << "', '" - << header.value().getStringView() << "'\n"; - return HeaderMap::Iterate::Continue; - }, - &iterate_data); - } + /** + * Dump the header map to the ostream specified + * + * @param os the stream to dump state to + * @param indent_level the depth, for pretty-printing. + * + * This function is called on Envoy fatal errors so should avoid memory allocation where possible. + */ + virtual void dumpState(std::ostream& os, int indent_level = 0) const PURE; /** * Allow easy pretty-printing of the key/value pairs in HeaderMap @@ -546,7 +541,7 @@ class HeaderMap { * @param headers the headers to print. */ friend std::ostream& operator<<(std::ostream& os, const HeaderMap& headers) { - headers.logState(os); + headers.dumpState(os); return os; } }; diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 40cee9aef4c9..6ace52dbb343 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -109,10 +109,10 @@ envoy_cc_library( deps = [":assert_lib"], ) -# Contains macros and helpers for logState utilities +# Contains macros and helpers for dumpState utilities envoy_cc_library( - name = "log_state_utils", - hdrs = ["log_state_utils.h"], + name = "dump_state_utils", + hdrs = ["dump_state_utils.h"], ) # Contains minimal code for logging to stderr. @@ -146,7 +146,7 @@ envoy_cc_library( srcs = ["logger_delegates.cc"], hdrs = ["logger_delegates.h"], deps = [ - ":log_state_utils", + ":dump_state_utils", ":macros", ":minimal_logger_lib", "//include/envoy/access_log:access_log_interface", diff --git a/source/common/common/log_state_utils.h b/source/common/common/dump_state_utils.h similarity index 83% rename from source/common/common/log_state_utils.h rename to source/common/common/dump_state_utils.h index 9f88becb8e55..8ef657b9571e 100644 --- a/source/common/common/log_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -4,20 +4,22 @@ namespace Envoy { -#define LOG_MEMBER(member) ", " #member ": " << (member) +// A collection of macros for pretty printing objects on fatal error. -#define LOG_OPTIONAL_MEMBER(member) \ +#define DUMP_MEMBER(member) ", " #member ": " << (member) + +#define DUMP_OPTIONAL_MEMBER(member) \ ", " #member ": " << ((member).has_value() ? absl::StrCat((member).value()) : "null") // Macro assumes local member variables // os (ostream) // indent_level (int) -#define LOG_DETAILS(member) \ +#define DUMP_DETAILS(member) \ do { \ os << spaces << #member ": "; \ if ((member) != nullptr) { \ os << "\n"; \ - (member)->logState(os, indent_level + 1); \ + (member)->dumpState(os, indent_level + 1); \ } else { \ os << spaces << "null\n"; \ } \ diff --git a/source/common/event/BUILD b/source/common/event/BUILD index d7ed37e65c04..12b235170ee9 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -75,7 +75,7 @@ envoy_cc_library( "//include/envoy/network:connection_handler_interface", "//source/common/common:minimal_logger_lib", "//source/common/common:thread_lib", - "//source/exe:crash_handler_lib", + "//source/exe:fatal_error_handler_lib", ] + select({ "//bazel:disable_signal_trace": [], "//conditions:default": [ diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index b33842b807b3..99300804d46f 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -43,13 +43,13 @@ DispatcherImpl::DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& post_timer_(createTimerInternal([this]() -> void { runPostCallbacks(); })), current_to_delete_(&to_delete_1_) { #ifdef ENVOY_HANDLE_SIGNALS - SignalAction::registerCrashHandler(*this); + SignalAction::registerFatalErrorHandler(*this); #endif } DispatcherImpl::~DispatcherImpl() { #ifdef ENVOY_HANDLE_SIGNALS - SignalAction::removeCrashHandler(*this); + SignalAction::removeFatalErrorHandler(*this); #endif } diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index ab278fdd7049..3bd2a1220c02 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -19,7 +19,7 @@ #include "common/event/libevent.h" #include "common/event/libevent_scheduler.h" -#include "exe/crash_handler.h" +#include "exe/fatal_error_handler.h" namespace Envoy { namespace Event { @@ -29,7 +29,7 @@ namespace Event { */ class DispatcherImpl : Logger::Loggable, public Dispatcher, - public CrashHandlerInterface { + public FatalErrorHandlerInterface { public: DispatcherImpl(Api::Api& api, Event::TimeSystem& time_system); DispatcherImpl(Buffer::WatermarkFactoryPtr&& factory, Api::Api& api, @@ -76,11 +76,11 @@ class DispatcherImpl : Logger::Loggable, return return_object; } - // CrashHandlerInterface - void crashHandler() const override { + // FatalErrorInterface + void onFatalError() const override { if (isThreadSafe()) { if (current_object_) { - current_object_->logState(std::cerr); + current_object_->dumpState(std::cerr); } } } diff --git a/source/common/http/BUILD b/source/common/http/BUILD index 16be9f55297f..ce3737bef977 100644 --- a/source/common/http/BUILD +++ b/source/common/http/BUILD @@ -172,10 +172,10 @@ envoy_cc_library( "//source/common/access_log:access_log_formatter_lib", "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", + "//source/common/common:dump_state_utils", "//source/common/common:empty_string", "//source/common/common:enum_to_int", "//source/common/common:linked_object", - "//source/common/common:log_state_utils", "//source/common/common:scope_tracker", "//source/common/common:utility_lib", "//source/common/http/http1:codec_lib", @@ -217,6 +217,7 @@ envoy_cc_library( ":headers_lib", "//include/envoy/http:header_map_interface", "//source/common/common:assert_lib", + "//source/common/common:dump_state_utils", "//source/common/common:empty_string", "//source/common/common:non_copyable", "//source/common/common:utility_lib", diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index 12046c4e6187..044abe060c7e 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -28,8 +28,8 @@ #include "envoy/upstream/upstream.h" #include "common/buffer/watermark_buffer.h" +#include "common/common/dump_state_utils.h" #include "common/common/linked_object.h" -#include "common/common/log_state_utils.h" #include "common/grpc/common.h" #include "common/http/conn_manager_config.h" #include "common/http/user_agent.h" @@ -421,17 +421,17 @@ class ConnectionManagerImpl : Logger::Loggable, bool verbose() const override; // ScopeTrackedObject - void logState(std::ostream& os, int indent_level = 0) const override { + void dumpState(std::ostream& os, int indent_level = 0) const override { const char* spaces = spacesForLevel(indent_level); - os << spaces << "ActiveStream " << this << LOG_MEMBER(stream_id_) - << LOG_MEMBER(has_continue_headers_) << LOG_MEMBER(is_head_request_) - << LOG_MEMBER(decoding_headers_only_) << LOG_MEMBER(encoding_headers_only_) << "\n"; - - LOG_DETAILS(request_headers_); - LOG_DETAILS(request_trailers_); - LOG_DETAILS(response_headers_); - LOG_DETAILS(response_trailers_); - LOG_DETAILS(&stream_info_); + os << spaces << "ActiveStream " << this << DUMP_MEMBER(stream_id_) + << DUMP_MEMBER(has_continue_headers_) << DUMP_MEMBER(is_head_request_) + << DUMP_MEMBER(decoding_headers_only_) << DUMP_MEMBER(encoding_headers_only_) << "\n"; + + DUMP_DETAILS(request_headers_); + DUMP_DETAILS(request_trailers_); + DUMP_DETAILS(response_headers_); + DUMP_DETAILS(response_trailers_); + DUMP_DETAILS(&stream_info_); } void traceRequest(); @@ -512,7 +512,7 @@ class ConnectionManagerImpl : Logger::Loggable, bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); } friend std::ostream& operator<<(std::ostream& os, const ActiveStream& s) { - s.logState(os); + s.dumpState(os); return os; } diff --git a/source/common/http/header_map_impl.cc b/source/common/http/header_map_impl.cc index 90ed21144ddb..d2389bfe4375 100644 --- a/source/common/http/header_map_impl.cc +++ b/source/common/http/header_map_impl.cc @@ -6,6 +6,7 @@ #include #include "common/common/assert.h" +#include "common/common/dump_state_utils.h" #include "common/common/empty_string.h" #include "common/common/utility.h" #include "common/singleton/const_singleton.h" @@ -554,6 +555,20 @@ void HeaderMapImpl::removePrefix(const LowerCaseString& prefix) { }); } +void HeaderMapImpl::dumpState(std::ostream& os, int indent_level) const { + using IterateData = std::pair; + const char* spaces = spacesForLevel(indent_level); + IterateData iterate_data = std::make_pair(&os, spaces); + iterate( + [](const HeaderEntry& header, void* context) -> HeaderMap::Iterate { + auto* data = static_cast(context); + *data->first << data->second << "'" << header.key().getStringView() << "', '" + << header.value().getStringView() << "'\n"; + return HeaderMap::Iterate::Continue; + }, + &iterate_data); +} + HeaderMapImpl::HeaderEntryImpl& HeaderMapImpl::maybeCreateInline(HeaderEntryImpl** entry, const LowerCaseString& key) { if (*entry) { diff --git a/source/common/http/header_map_impl.h b/source/common/http/header_map_impl.h index 51b319499722..f7d3a66937f8 100644 --- a/source/common/http/header_map_impl.h +++ b/source/common/http/header_map_impl.h @@ -81,6 +81,7 @@ class HeaderMapImpl : public HeaderMap, NonCopyable { void removePrefix(const LowerCaseString& key) override; size_t size() const override { return headers_.size(); } bool empty() const override { return headers_.empty(); } + void dumpState(std::ostream& os, int indent_level = 0) const override; protected: // For tests only, unoptimized, they aren't intended for regular HeaderMapImpl users. diff --git a/source/common/stream_info/BUILD b/source/common/stream_info/BUILD index 006265a9ac5a..b80bdc6d4c06 100644 --- a/source/common/stream_info/BUILD +++ b/source/common/stream_info/BUILD @@ -15,7 +15,7 @@ envoy_cc_library( ":filter_state_lib", "//include/envoy/stream_info:stream_info_interface", "//source/common/common:assert_lib", - "//source/common/common:log_state_utils", + "//source/common/common:dump_state_utils", ], ) diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index af4ee647a2ec..b434991af40b 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -7,7 +7,7 @@ #include "envoy/stream_info/stream_info.h" #include "common/common/assert.h" -#include "common/common/log_state_utils.h" +#include "common/common/dump_state_utils.h" #include "common/stream_info/filter_state_impl.h" namespace Envoy { @@ -209,11 +209,11 @@ struct StreamInfoImpl : public StreamInfo { return upstream_transport_failure_reason_; } - void logState(std::ostream& os, int indent_level = 0) const { + void dumpState(std::ostream& os, int indent_level = 0) const { const char* spaces = spacesForLevel(indent_level); - os << spaces << "StreamInfoImpl " << this << LOG_OPTIONAL_MEMBER(protocol_) - << LOG_OPTIONAL_MEMBER(response_code_) << LOG_OPTIONAL_MEMBER(response_code_details_) - << LOG_MEMBER(health_check_request_) << LOG_MEMBER(route_name_) << "\n"; + os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(protocol_) + << DUMP_OPTIONAL_MEMBER(response_code_) << DUMP_OPTIONAL_MEMBER(response_code_details_) + << DUMP_MEMBER(health_check_request_) << DUMP_MEMBER(route_name_) << "\n"; } TimeSource& time_source_; diff --git a/source/exe/BUILD b/source/exe/BUILD index 5ec455be09aa..9d1425032a26 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -31,8 +31,8 @@ envoy_cc_binary( ) envoy_cc_library( - name = "crash_handler_lib", - hdrs = ["crash_handler.h"], + name = "fatal_error_handler_lib", + hdrs = ["fatal_error_handler.h"], ) envoy_cc_library( @@ -127,7 +127,7 @@ envoy_cc_library( hdrs = ["signal_action.h"], tags = ["backtrace"], deps = [ - ":crash_handler_lib", + ":fatal_error_handler_lib", "//source/common/common:assert_lib", "//source/common/common:non_copyable", "//source/common/singleton:threadsafe_singleton", diff --git a/source/exe/crash_handler.h b/source/exe/fatal_error_handler.h similarity index 71% rename from source/exe/crash_handler.h rename to source/exe/fatal_error_handler.h index 485b2de8da21..95c185911d3e 100644 --- a/source/exe/crash_handler.h +++ b/source/exe/fatal_error_handler.h @@ -9,10 +9,10 @@ namespace Envoy { // // This is split out of signal_action.h because it is exempted from various // builds. -class CrashHandlerInterface { +class FatalErrorHandlerInterface { public: - virtual ~CrashHandlerInterface() = default; - virtual void crashHandler() const PURE; + virtual ~FatalErrorHandlerInterface() = default; + virtual void onFatalError() const PURE; }; } // namespace Envoy diff --git a/source/exe/signal_action.cc b/source/exe/signal_action.cc index 344b11b4d541..56a0e01be04b 100644 --- a/source/exe/signal_action.cc +++ b/source/exe/signal_action.cc @@ -9,27 +9,27 @@ namespace Envoy { ABSL_CONST_INIT static absl::Mutex failure_mutex(absl::kConstInit); -using FailureFunctionList = std::list; -ABSL_CONST_INIT std::atomic crash_handlers{nullptr}; +using FailureFunctionList = std::list; +ABSL_CONST_INIT std::atomic fatal_error_handlers{nullptr}; -void SignalAction::registerCrashHandler(const CrashHandlerInterface& handler) { +void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& handler) { absl::MutexLock l(&failure_mutex); - FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); if (list == nullptr) { list = new FailureFunctionList; } list->push_back(&handler); - crash_handlers.store(list, std::memory_order_release); + fatal_error_handlers.store(list, std::memory_order_release); } -void SignalAction::removeCrashHandler(const CrashHandlerInterface& handler) { +void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& handler) { absl::MutexLock l(&failure_mutex); - FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); list->remove(&handler); if (list->empty()) { delete list; } else { - crash_handlers.store(list, std::memory_order_release); + fatal_error_handlers.store(list, std::memory_order_release); } } @@ -46,11 +46,11 @@ void SignalAction::sigHandler(int sig, siginfo_t* info, void* context) { } tracer.logTrace(); - FailureFunctionList* list = crash_handlers.exchange(nullptr, std::memory_order_relaxed); + FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); if (list) { // Finally after logging the stack trace, call any registered crash handlers. for (const auto* handler : *list) { - handler->crashHandler(); + handler->onFatalError(); } } diff --git a/source/exe/signal_action.h b/source/exe/signal_action.h index 15354e166dec..de2cad770e1e 100644 --- a/source/exe/signal_action.h +++ b/source/exe/signal_action.h @@ -8,7 +8,7 @@ #include "common/common/non_copyable.h" -#include "exe/crash_handler.h" +#include "exe/fatal_error_handler.h" #include "server/backtrace.h" @@ -78,13 +78,13 @@ class SignalAction : NonCopyable { * Add this handler to the list of functions which will be called if Envoy * receives a fatal signal. */ - static void registerCrashHandler(const CrashHandlerInterface& handler); + static void registerFatalErrorHandler(const FatalErrorHandlerInterface& handler); /** * Removes this handler from the list of functions which will be called if Envoy * receives a fatal signal. */ - static void removeCrashHandler(const CrashHandlerInterface& handler); + static void removeFatalErrorHandler(const FatalErrorHandlerInterface& handler); private: /** @@ -143,7 +143,7 @@ class SignalAction : NonCopyable { char* altstack_{}; std::array previous_handlers_; stack_t previous_altstack_; - std::list crash_handlers_; + std::list fatal_error_handlers_; }; } // namespace Envoy diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 036a3ed64ef4..ce4e8af46284 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -4320,7 +4320,7 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) { .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* { ASSERT(object != nullptr); // On the first call, this should be the active stream. std::stringstream out; - object->logState(out); + object->dumpState(out); std::string state = out.str(); EXPECT_THAT(state, testing::HasSubstr("request_headers_: null")); EXPECT_THAT(state, testing::HasSubstr("protocol_: 1")); @@ -4342,7 +4342,7 @@ TEST_F(HttpConnectionManagerImplTest, TestSessionTrace) { .WillOnce(Invoke([](const ScopeTrackedObject* object) -> const ScopeTrackedObject* { ASSERT(object != nullptr); // On the first call, this should be the active stream. std::stringstream out; - object->logState(out); + object->dumpState(out); std::string state = out.str(); EXPECT_THAT(state, testing::HasSubstr("request_headers_: \n")); EXPECT_THAT(state, testing::HasSubstr("':authority', 'host'\n")); diff --git a/test/exe/signals_test.cc b/test/exe/signals_test.cc index 4cb533fae2c4..2b71cbf1c5e0 100644 --- a/test/exe/signals_test.cc +++ b/test/exe/signals_test.cc @@ -34,13 +34,13 @@ TEST(SignalsDeathTest, InvalidAddressDeathTest) { "backtrace.*Segmentation fault"); } -class TestCrashHandler : public CrashHandlerInterface { - virtual void crashHandler() const override { std::cerr << "HERE!"; } +class TestFatalErrorHandler : public FatalErrorHandlerInterface { + virtual void onFatalError() const override { std::cerr << "HERE!"; } }; TEST(SignalsDeathTest, RegisteredHandlerTest) { - TestCrashHandler handler; - SignalAction::registerCrashHandler(handler); + TestFatalErrorHandler handler; + SignalAction::registerFatalErrorHandler(handler); SignalAction actions; EXPECT_DEATH_LOG_TO_STDERR( []() -> void { @@ -49,7 +49,7 @@ TEST(SignalsDeathTest, RegisteredHandlerTest) { *(nasty_ptr) = 0; }(), "backtrace.*Segmentation fault"); - SignalAction::removeCrashHandler(handler); + SignalAction::removeFatalErrorHandler(handler); /* SignalAction actions; EXPECT_DEATH_LOG_TO_STDERR( From ce554b703db5985d812226fae499d0cb4aebff6e Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 27 Jun 2019 10:24:58 -0400 Subject: [PATCH 06/11] more detail Signed-off-by: Alyssa Wilk --- source/common/common/dump_state_utils.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index 8ef657b9571e..ad9ad76a741a 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -5,6 +5,9 @@ namespace Envoy { // A collection of macros for pretty printing objects on fatal error. +// These are fairly ugly in an attempt to maximize the conditions where fatal error logging occurs, +// i.e. under the Envoy signal handler if encountering a SIGSEVG due to OOM, where allocating more +// memory would likely lead to the crash handler itself causing a subeqeuent OOM. #define DUMP_MEMBER(member) ", " #member ": " << (member) From 42e27ba108d61ff70033974b42a6d5e6f0db3ce9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 1 Jul 2019 17:01:55 -0400 Subject: [PATCH 07/11] some comments: still no config guards Signed-off-by: Alyssa Wilk --- include/envoy/common/scope_tracker.h | 2 +- include/envoy/event/dispatcher.h | 2 ++ source/common/event/dispatcher_impl.h | 2 ++ source/exe/signal_action.cc | 7 +++++++ test/common/stream_info/stream_info_impl_test.cc | 14 ++++++++++++++ test/exe/signals_test.cc | 12 ++---------- 6 files changed, 28 insertions(+), 11 deletions(-) diff --git a/include/envoy/common/scope_tracker.h b/include/envoy/common/scope_tracker.h index 626e6a2bac6b..eb72edf3b7af 100644 --- a/include/envoy/common/scope_tracker.h +++ b/include/envoy/common/scope_tracker.h @@ -8,7 +8,7 @@ namespace Envoy { /* * A class for tracking the scope of work. - * Currently this is only used for best-effort tracking the any L7 stream doing + * Currently this is only used for best-effort tracking the L7 stream doing * work if a fatal error occurs. */ class ScopeTrackedObject { diff --git a/include/envoy/event/dispatcher.h b/include/envoy/event/dispatcher.h index 4b9ffbf4e0ee..4dbe2ab06e16 100644 --- a/include/envoy/event/dispatcher.h +++ b/include/envoy/event/dispatcher.h @@ -206,6 +206,8 @@ class 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. * + * 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. */ virtual const ScopeTrackedObject* setTrackedObject(const ScopeTrackedObject* object) PURE; diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 3bd2a1220c02..e76579962a7d 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -78,6 +78,8 @@ class DispatcherImpl : Logger::Loggable, // FatalErrorInterface void onFatalError() const override { + // Dump the stae 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(std::cerr); diff --git a/source/exe/signal_action.cc b/source/exe/signal_action.cc index 56a0e01be04b..40e964a42a7d 100644 --- a/source/exe/signal_action.cc +++ b/source/exe/signal_action.cc @@ -9,6 +9,13 @@ namespace Envoy { ABSL_CONST_INIT static absl::Mutex failure_mutex(absl::kConstInit); +// Since we can't grab the failure mutex on fatal error (snagging locks under +// fatal crash causing potential deadlocks) access the handler list as an atomic +// operation, to minimize the chance that one thread is operating on the list +// while the crash handler is attempting to access it. +// This basically makes edits to the list thread-safe - if one thread is +// modifying the list rather than crashing in the crash handler due to accessing +// the list in a non-thread-safe manner, it simply won't log crash traces. using FailureFunctionList = std::list; ABSL_CONST_INIT std::atomic fatal_error_handlers{nullptr}; diff --git a/test/common/stream_info/stream_info_impl_test.cc b/test/common/stream_info/stream_info_impl_test.cc index fec622ddc810..380f60ffcebb 100644 --- a/test/common/stream_info/stream_info_impl_test.cc +++ b/test/common/stream_info/stream_info_impl_test.cc @@ -201,6 +201,20 @@ TEST_F(StreamInfoImplTest, DynamicMetadataTest) { EXPECT_TRUE(json.find("\"another_key\":\"another_value\"") != std::string::npos); } +TEST_F(StreamInfoImplTest, DumpStateTest) { + StreamInfoImpl stream_info(Http::Protocol::Http2, test_time_.timeSystem()); + std::string prefix = ""; + + for (int i = 0; i < 7; ++i) { + std::stringstream out; + stream_info.dumpState(out, i); + std::string state = out.str(); + EXPECT_TRUE(absl::StartsWith(state, prefix)); + EXPECT_THAT(state, testing::HasSubstr("protocol_: 2")); + prefix = prefix + " "; + } +} + } // namespace } // namespace StreamInfo } // namespace Envoy diff --git a/test/exe/signals_test.cc b/test/exe/signals_test.cc index 2b71cbf1c5e0..158d3b804cf3 100644 --- a/test/exe/signals_test.cc +++ b/test/exe/signals_test.cc @@ -42,23 +42,15 @@ TEST(SignalsDeathTest, RegisteredHandlerTest) { TestFatalErrorHandler handler; SignalAction::registerFatalErrorHandler(handler); SignalAction actions; + // Make sure the cerr "HERE" registered above is logged on fatal error. EXPECT_DEATH_LOG_TO_STDERR( []() -> void { // Oops! volatile int* nasty_ptr = reinterpret_cast(0x0); *(nasty_ptr) = 0; }(), - "backtrace.*Segmentation fault"); + "HERE"); SignalAction::removeFatalErrorHandler(handler); - - /* SignalAction actions; - EXPECT_DEATH_LOG_TO_STDERR( - []() -> void { - // Oops! - volatile int* nasty_ptr = reinterpret_cast(0x0); - *(nasty_ptr) = 0; - }(), - "HERE!");*/ } TEST(SignalsDeathTest, BusDeathTest) { From 41d620a73be3b3ceabb330b2b5b0e6d7a7d64745 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 2 Jul 2019 09:37:42 -0400 Subject: [PATCH 08/11] renames, file moves, relnotes, build flags Signed-off-by: Alyssa Wilk --- docs/root/intro/version_history.rst | 1 + source/common/common/scope_tracker.h | 11 ++++++-- source/common/event/BUILD | 4 +-- source/common/event/dispatcher_impl.cc | 2 +- source/common/event/dispatcher_impl.h | 3 +- source/common/http/conn_manager_impl.cc | 15 ++++++---- source/common/signal/BUILD | 28 +++++++++++++++++++ .../signal}/fatal_error_handler.h | 0 .../{exe => common/signal}/signal_action.cc | 6 +++- source/{exe => common/signal}/signal_action.h | 3 +- source/exe/BUILD | 21 +------------- source/exe/main_common.h | 2 +- test/BUILD | 2 +- test/common/signal/BUILD | 19 +++++++++++++ test/{exe => common/signal}/signals_test.cc | 2 +- test/exe/BUILD | 10 ------- test/exe/main_common_test.cc | 2 +- test/main.cc | 2 +- 18 files changed, 82 insertions(+), 51 deletions(-) create mode 100644 source/common/signal/BUILD rename source/{exe => common/signal}/fatal_error_handler.h (100%) rename source/{exe => common/signal}/signal_action.cc (97%) rename source/{exe => common/signal}/signal_action.h (99%) create mode 100644 test/common/signal/BUILD rename test/{exe => common/signal}/signals_test.cc (99%) diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index b427780db79e..63d359b5c01a 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -35,6 +35,7 @@ Version history * http: added support for :ref:`preserve_external_request_id` that represents whether the x-request-id should not be reset on edge entry inside mesh * http: changed `sendLocalReply` to send percent-encoded `GrpcMessage`. * http: added :ref:`dynamic forward proxy ` support. +* http: tracking the active stream and dumping state in Envoy crash handlers. This can be disabled by building with -DENVOY_SKIP_OBJECT_TRACE_ON_DUMP * jwt_authn: make filter's parsing of JWT more flexible, allowing syntax like ``jwt=eyJhbGciOiJS...ZFnFIw,extra=7,realm=123`` * listener: added :ref:`source IP ` and :ref:`source port ` filter diff --git a/source/common/common/scope_tracker.h b/source/common/common/scope_tracker.h index 446f0a1a8487..bed58c3fa8c0 100644 --- a/source/common/common/scope_tracker.h +++ b/source/common/common/scope_tracker.h @@ -5,14 +5,19 @@ namespace Envoy { -class ScopeTrackerImpl { +// A small class for tracking the scope of the 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. +class ScopeTrackerScopeState { public: - ScopeTrackerImpl(const ScopeTrackedObject* object, Event::Dispatcher& dispatcher) + ScopeTrackerScopeState(const ScopeTrackedObject* object, Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) { latched_object_ = dispatcher_.setTrackedObject(object); } - ~ScopeTrackerImpl() { dispatcher_.setTrackedObject(latched_object_); } + ~ScopeTrackerScopeState() { dispatcher_.setTrackedObject(latched_object_); } private: const ScopeTrackedObject* latched_object_; diff --git a/source/common/event/BUILD b/source/common/event/BUILD index 12b235170ee9..78461dcd3181 100644 --- a/source/common/event/BUILD +++ b/source/common/event/BUILD @@ -75,11 +75,11 @@ envoy_cc_library( "//include/envoy/network:connection_handler_interface", "//source/common/common:minimal_logger_lib", "//source/common/common:thread_lib", - "//source/exe:fatal_error_handler_lib", + "//source/common/signal:fatal_error_handler_lib", ] + select({ "//bazel:disable_signal_trace": [], "//conditions:default": [ - "//source/exe:sigaction_lib", + "//source/common/signal:sigaction_lib", ], }), ) diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 99300804d46f..bbf88a0c1296 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -26,7 +26,7 @@ #include "event2/event.h" #ifdef ENVOY_HANDLE_SIGNALS -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #endif namespace Envoy { diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index e76579962a7d..16939bb16409 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -18,8 +18,7 @@ #include "common/common/thread.h" #include "common/event/libevent.h" #include "common/event/libevent_scheduler.h" - -#include "exe/fatal_error_handler.h" +#include "common/signal/fatal_error_handler.h" namespace Envoy { namespace Event { diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 32de255fbc10..8e905ac5aba0 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -417,7 +417,8 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect connection_manager_.stats_.named_.downstream_rq_time_, connection_manager_.timeSource())), stream_info_(connection_manager_.codec_->protocol(), connection_manager_.timeSource()), upstream_options_(std::make_shared()) { - ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + ScopeTrackerScopeState scope(this, + connection_manager_.read_callbacks_->connection().dispatcher()); connection_manager_.stats_.named_.downstream_rq_total_.inc(); connection_manager_.stats_.named_.downstream_rq_active_.inc(); @@ -588,7 +589,8 @@ const Network::Connection* ConnectionManagerImpl::ActiveStream::connection() { // TODO(alyssawilk) all the calls here should be audited for order priority, // e.g. many early returns do not currently handle connection: close properly. void ConnectionManagerImpl::ActiveStream::decodeHeaders(HeaderMapPtr&& headers, bool end_stream) { - ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + ScopeTrackerScopeState scope(this, + connection_manager_.read_callbacks_->connection().dispatcher()); request_headers_ = std::move(headers); if (Http::Headers::get().MethodValues.Head == request_headers_->Method()->value().getStringView()) { @@ -885,7 +887,8 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(ActiveStreamDecoderFilte } void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, bool end_stream) { - ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + ScopeTrackerScopeState scope(this, + connection_manager_.read_callbacks_->connection().dispatcher()); maybeEndDecode(end_stream); stream_info_.addBytesReceived(data.length()); @@ -895,7 +898,8 @@ void ConnectionManagerImpl::ActiveStream::decodeData(Buffer::Instance& data, boo void ConnectionManagerImpl::ActiveStream::decodeData( ActiveStreamDecoderFilter* filter, Buffer::Instance& data, bool end_stream, FilterIterationStartState filter_iteration_start_state) { - ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + ScopeTrackerScopeState scope(this, + connection_manager_.read_callbacks_->connection().dispatcher()); resetIdleTimer(); // If we previously decided to decode only the headers, do nothing here. @@ -1035,7 +1039,8 @@ void ConnectionManagerImpl::ActiveStream::addDecodedData(ActiveStreamDecoderFilt } void ConnectionManagerImpl::ActiveStream::decodeTrailers(HeaderMapPtr&& trailers) { - ScopeTrackerImpl scope(this, connection_manager_.read_callbacks_->connection().dispatcher()); + ScopeTrackerScopeState scope(this, + connection_manager_.read_callbacks_->connection().dispatcher()); resetIdleTimer(); maybeEndDecode(true); request_trailers_ = std::move(trailers); diff --git a/source/common/signal/BUILD b/source/common/signal/BUILD new file mode 100644 index 000000000000..17dec6c9be55 --- /dev/null +++ b/source/common/signal/BUILD @@ -0,0 +1,28 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) + +envoy_package() + +envoy_cc_library( + name = "fatal_error_handler_lib", + hdrs = ["fatal_error_handler.h"], +) + +envoy_cc_library( + name = "sigaction_lib", + srcs = ["signal_action.cc"], + hdrs = ["signal_action.h"], + tags = ["backtrace"], + deps = [ + ":fatal_error_handler_lib", + "//source/common/common:assert_lib", + "//source/common/common:non_copyable", + "//source/common/singleton:threadsafe_singleton", + "//source/server:backtrace_lib", + ], +) diff --git a/source/exe/fatal_error_handler.h b/source/common/signal/fatal_error_handler.h similarity index 100% rename from source/exe/fatal_error_handler.h rename to source/common/signal/fatal_error_handler.h diff --git a/source/exe/signal_action.cc b/source/common/signal/signal_action.cc similarity index 97% rename from source/exe/signal_action.cc rename to source/common/signal/signal_action.cc index 40e964a42a7d..d39611523541 100644 --- a/source/exe/signal_action.cc +++ b/source/common/signal/signal_action.cc @@ -1,4 +1,4 @@ -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #include @@ -20,6 +20,7 @@ using FailureFunctionList = std::list; ABSL_CONST_INIT std::atomic fatal_error_handlers{nullptr}; void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& handler) { +#ifndef ENVOY_SKIP_OBJECT_TRACE_ON_DUMP absl::MutexLock l(&failure_mutex); FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); if (list == nullptr) { @@ -27,9 +28,11 @@ void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& h } list->push_back(&handler); fatal_error_handlers.store(list, std::memory_order_release); +#endif } void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& handler) { +#ifndef ENVOY_SKIP_OBJECT_TRACE_ON_DUMP absl::MutexLock l(&failure_mutex); FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); list->remove(&handler); @@ -38,6 +41,7 @@ void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& han } else { fatal_error_handlers.store(list, std::memory_order_release); } +#endif } constexpr int SignalAction::FATAL_SIGS[]; diff --git a/source/exe/signal_action.h b/source/common/signal/signal_action.h similarity index 99% rename from source/exe/signal_action.h rename to source/common/signal/signal_action.h index de2cad770e1e..12fcb1805a7b 100644 --- a/source/exe/signal_action.h +++ b/source/common/signal/signal_action.h @@ -7,8 +7,7 @@ #include #include "common/common/non_copyable.h" - -#include "exe/fatal_error_handler.h" +#include "common/signal/fatal_error_handler.h" #include "server/backtrace.h" diff --git a/source/exe/BUILD b/source/exe/BUILD index 9d1425032a26..2f418142d71c 100644 --- a/source/exe/BUILD +++ b/source/exe/BUILD @@ -30,11 +30,6 @@ envoy_cc_binary( deps = ["envoy_main_entry_lib"], ) -envoy_cc_library( - name = "fatal_error_handler_lib", - hdrs = ["fatal_error_handler.h"], -) - envoy_cc_library( name = "envoy_common_lib", deps = [ @@ -81,7 +76,7 @@ envoy_cc_library( ] + select({ "//bazel:disable_signal_trace": [], "//conditions:default": [ - ":sigaction_lib", + "//source/common/signal:sigaction_lib", ":terminate_handler_lib", ], }) + envoy_cc_platform_dep("platform_impl_lib"), @@ -121,20 +116,6 @@ envoy_cc_win32_library( ], ) -envoy_cc_library( - name = "sigaction_lib", - srcs = ["signal_action.cc"], - hdrs = ["signal_action.h"], - tags = ["backtrace"], - deps = [ - ":fatal_error_handler_lib", - "//source/common/common:assert_lib", - "//source/common/common:non_copyable", - "//source/common/singleton:threadsafe_singleton", - "//source/server:backtrace_lib", - ], -) - envoy_cc_library( name = "terminate_handler_lib", srcs = ["terminate_handler.cc"], diff --git a/source/exe/main_common.h b/source/exe/main_common.h index 3a49d8979099..a8944fd69b34 100644 --- a/source/exe/main_common.h +++ b/source/exe/main_common.h @@ -17,7 +17,7 @@ #include "server/server.h" #ifdef ENVOY_HANDLE_SIGNALS -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #include "exe/terminate_handler.h" #endif diff --git a/test/BUILD b/test/BUILD index fa80bf514d36..4c2e42990737 100644 --- a/test/BUILD +++ b/test/BUILD @@ -38,6 +38,6 @@ envoy_cc_test_library( "//test/test_common:printers_lib", ] + select({ "//bazel:disable_signal_trace": [], - "//conditions:default": ["//source/exe:sigaction_lib"], + "//conditions:default": ["//source/common/signal:sigaction_lib"], }), ) diff --git a/test/common/signal/BUILD b/test/common/signal/BUILD new file mode 100644 index 000000000000..c29cfd2a56b3 --- /dev/null +++ b/test/common/signal/BUILD @@ -0,0 +1,19 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_test", + "envoy_package", +) + +envoy_package() + +envoy_cc_test( + name = "signals_test", + srcs = ["signals_test.cc"], + tags = ["backtrace"], + deps = [ + "//source/common/signal:sigaction_lib", + "//test/test_common:utility_lib", + ], +) diff --git a/test/exe/signals_test.cc b/test/common/signal/signals_test.cc similarity index 99% rename from test/exe/signals_test.cc rename to test/common/signal/signals_test.cc index 158d3b804cf3..633c0ce4a3ff 100644 --- a/test/exe/signals_test.cc +++ b/test/common/signal/signals_test.cc @@ -1,7 +1,7 @@ #include #include -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #include "test/test_common/utility.h" diff --git a/test/exe/BUILD b/test/exe/BUILD index 641b1c9efc60..12235850ca35 100644 --- a/test/exe/BUILD +++ b/test/exe/BUILD @@ -52,16 +52,6 @@ envoy_cc_test( ], ) -envoy_cc_test( - name = "signals_test", - srcs = ["signals_test.cc"], - tags = ["backtrace"], - deps = [ - "//source/exe:sigaction_lib", - "//test/test_common:utility_lib", - ], -) - envoy_cc_test( name = "terminate_handler_test", srcs = ["terminate_handler_test.cc"], diff --git a/test/exe/main_common_test.cc b/test/exe/main_common_test.cc index 44c693a91f8e..baeeb243976a 100644 --- a/test/exe/main_common_test.cc +++ b/test/exe/main_common_test.cc @@ -16,7 +16,7 @@ #include "gtest/gtest.h" #ifdef ENVOY_HANDLE_SIGNALS -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #endif #include "absl/synchronization/notification.h" diff --git a/test/main.cc b/test/main.cc index 83a9d2b72c9f..b7295edd0b02 100644 --- a/test/main.cc +++ b/test/main.cc @@ -8,7 +8,7 @@ #include "absl/debugging/symbolize.h" #ifdef ENVOY_HANDLE_SIGNALS -#include "exe/signal_action.h" +#include "common/signal/signal_action.h" #endif // The main entry point (and the rest of this file) should have no logic in it, From e8bbe344b8ee9630e3ea3e9785982dc732a1e144 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 2 Jul 2019 10:53:10 -0400 Subject: [PATCH 09/11] speeling Signed-off-by: Alyssa Wilk --- source/common/common/dump_state_utils.h | 4 ++-- source/common/event/dispatcher_impl.h | 2 +- test/common/signal/signals_test.cc | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/common/dump_state_utils.h b/source/common/common/dump_state_utils.h index ad9ad76a741a..c17c7b6c2ec6 100644 --- a/source/common/common/dump_state_utils.h +++ b/source/common/common/dump_state_utils.h @@ -6,8 +6,8 @@ namespace Envoy { // A collection of macros for pretty printing objects on fatal error. // These are fairly ugly in an attempt to maximize the conditions where fatal error logging occurs, -// i.e. under the Envoy signal handler if encountering a SIGSEVG due to OOM, where allocating more -// memory would likely lead to the crash handler itself causing a subeqeuent OOM. +// i.e. under the Envoy signal handler if encountering a crash due to OOM, where allocating more +// memory would likely lead to the crash handler itself causing a subsequent OOM. #define DUMP_MEMBER(member) ", " #member ": " << (member) diff --git a/source/common/event/dispatcher_impl.h b/source/common/event/dispatcher_impl.h index 16939bb16409..62d365e7a197 100644 --- a/source/common/event/dispatcher_impl.h +++ b/source/common/event/dispatcher_impl.h @@ -77,7 +77,7 @@ class DispatcherImpl : Logger::Loggable, // FatalErrorInterface void onFatalError() const override { - // Dump the stae of the tracked object if it is in the current thread. This generally results + // 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_) { diff --git a/test/common/signal/signals_test.cc b/test/common/signal/signals_test.cc index 633c0ce4a3ff..72b28c7bbf8b 100644 --- a/test/common/signal/signals_test.cc +++ b/test/common/signal/signals_test.cc @@ -42,7 +42,7 @@ TEST(SignalsDeathTest, RegisteredHandlerTest) { TestFatalErrorHandler handler; SignalAction::registerFatalErrorHandler(handler); SignalAction actions; - // Make sure the cerr "HERE" registered above is logged on fatal error. + // Make sure the fatal error log "HERE" registered above is logged on fatal error. EXPECT_DEATH_LOG_TO_STDERR( []() -> void { // Oops! From 56ccb5a8ee3c31ea7e66c066dda1ab12f693c044 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 2 Jul 2019 14:02:50 -0400 Subject: [PATCH 10/11] better compile config Signed-off-by: Alyssa Wilk --- bazel/BUILD | 5 +++++ bazel/README.md | 1 + bazel/envoy_internal.bzl | 3 +++ docs/root/intro/version_history.rst | 2 +- source/common/signal/signal_action.cc | 4 ++++ 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/bazel/BUILD b/bazel/BUILD index c3ac13989d2a..f7b3ccca7648 100755 --- a/bazel/BUILD +++ b/bazel/BUILD @@ -100,6 +100,11 @@ config_setting( values = {"define": "signal_trace=disabled"}, ) +config_setting( + name = "disable_object_dump_on_signal_trace", + values = {"define": "object_dump_on_signal_trace=disabled"}, +) + config_setting( name = "disable_hot_restart", values = {"define": "hot_restart=disabled"}, diff --git a/bazel/README.md b/bazel/README.md index 0eba43900fa5..eb0d47eb61da 100644 --- a/bazel/README.md +++ b/bazel/README.md @@ -350,6 +350,7 @@ The following optional features can be disabled on the Bazel build command-line: * Hot restart with `--define hot_restart=disabled` * Google C++ gRPC client with `--define google_grpc=disabled` * Backtracing on signals with `--define signal_trace=disabled` +* Active stream state dump on signals with `--define signal_trace=disabled` or `--define disable_object_dump_on_signal_trace=disabled` * tcmalloc with `--define tcmalloc=disabled` ## Enabling optional features diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index f12d52b2570e..c25c17eeb958 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -49,6 +49,9 @@ def envoy_copts(repository, test = False): }) + select({ repository + "//bazel:disable_signal_trace": [], "//conditions:default": ["-DENVOY_HANDLE_SIGNALS"], + }) + select({ + repository + "//bazel:disable_object_dump_on_signal_trace": [], + "//conditions:default": ["-DENVOY_SKIP_OBJECT_TRACE_ON_DUMP"], }) + select({ repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"], "//conditions:default": [], diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 63d359b5c01a..9e561799886b 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -35,7 +35,7 @@ Version history * http: added support for :ref:`preserve_external_request_id` that represents whether the x-request-id should not be reset on edge entry inside mesh * http: changed `sendLocalReply` to send percent-encoded `GrpcMessage`. * http: added :ref:`dynamic forward proxy ` support. -* http: tracking the active stream and dumping state in Envoy crash handlers. This can be disabled by building with -DENVOY_SKIP_OBJECT_TRACE_ON_DUMP +* http: tracking the active stream and dumping state in Envoy crash handlers. This can be disabled by building with `--define disable_object_dump_on_signal_trace=disabled` * jwt_authn: make filter's parsing of JWT more flexible, allowing syntax like ``jwt=eyJhbGciOiJS...ZFnFIw,extra=7,realm=123`` * listener: added :ref:`source IP ` and :ref:`source port ` filter diff --git a/source/common/signal/signal_action.cc b/source/common/signal/signal_action.cc index d39611523541..79c79e5f4df9 100644 --- a/source/common/signal/signal_action.cc +++ b/source/common/signal/signal_action.cc @@ -28,6 +28,8 @@ void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& h } list->push_back(&handler); fatal_error_handlers.store(list, std::memory_order_release); +#else + UNREFERENCED_PARAMETER(handler); #endif } @@ -41,6 +43,8 @@ void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& han } else { fatal_error_handlers.store(list, std::memory_order_release); } +#else + UNREFERENCED_PARAMETER(handler); #endif } From f0b1519c9692556a9b15224fd519507f75ea30fb Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 3 Jul 2019 11:44:19 -0400 Subject: [PATCH 11/11] hopefully fixing build/copt inversion Signed-off-by: Alyssa Wilk --- bazel/envoy_internal.bzl | 2 +- source/common/signal/signal_action.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/envoy_internal.bzl b/bazel/envoy_internal.bzl index c25c17eeb958..41b86a4d1b0a 100644 --- a/bazel/envoy_internal.bzl +++ b/bazel/envoy_internal.bzl @@ -51,7 +51,7 @@ def envoy_copts(repository, test = False): "//conditions:default": ["-DENVOY_HANDLE_SIGNALS"], }) + select({ repository + "//bazel:disable_object_dump_on_signal_trace": [], - "//conditions:default": ["-DENVOY_SKIP_OBJECT_TRACE_ON_DUMP"], + "//conditions:default": ["-DENVOY_OBJECT_TRACE_ON_DUMP"], }) + select({ repository + "//bazel:enable_log_debug_assert_in_release": ["-DENVOY_LOG_DEBUG_ASSERT_IN_RELEASE"], "//conditions:default": [], diff --git a/source/common/signal/signal_action.cc b/source/common/signal/signal_action.cc index 79c79e5f4df9..edc9dca40be9 100644 --- a/source/common/signal/signal_action.cc +++ b/source/common/signal/signal_action.cc @@ -20,7 +20,7 @@ using FailureFunctionList = std::list; ABSL_CONST_INIT std::atomic fatal_error_handlers{nullptr}; void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& handler) { -#ifndef ENVOY_SKIP_OBJECT_TRACE_ON_DUMP +#ifdef ENVOY_OBJECT_TRACE_ON_DUMP absl::MutexLock l(&failure_mutex); FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); if (list == nullptr) { @@ -34,7 +34,7 @@ void SignalAction::registerFatalErrorHandler(const FatalErrorHandlerInterface& h } void SignalAction::removeFatalErrorHandler(const FatalErrorHandlerInterface& handler) { -#ifndef ENVOY_SKIP_OBJECT_TRACE_ON_DUMP +#ifdef ENVOY_OBJECT_TRACE_ON_DUMP absl::MutexLock l(&failure_mutex); FailureFunctionList* list = fatal_error_handlers.exchange(nullptr, std::memory_order_relaxed); list->remove(&handler);