From 1b8ae3b7f2d3618ae64eeb0f2f361acff8ff6f74 Mon Sep 17 00:00:00 2001 From: Elisha Ziskind Date: Fri, 13 Sep 2019 13:44:38 -0400 Subject: [PATCH 1/3] Return processContext as pointer instead of reference Signed-off-by: Elisha Ziskind --- include/envoy/server/filter_config.h | 5 +++-- include/envoy/server/instance.h | 2 +- source/server/config_validation/server.h | 2 +- source/server/listener_manager_impl.h | 2 +- source/server/server.h | 2 +- test/integration/filters/process_context_filter.cc | 2 +- test/mocks/server/mocks.h | 4 ++-- test/server/server_test.cc | 4 ++-- 8 files changed, 12 insertions(+), 11 deletions(-) diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 4864d5bf9da6..2739f2057f52 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -180,9 +180,10 @@ class FactoryContext : public virtual CommonFactoryContext { virtual Grpc::Context& grpcContext() PURE; /** - * @return ProcessContext& a reference to the process context. + * @return ProcessContext* a pointer to the process context. Will be null when running in + * validation mode. */ - virtual ProcessContext& processContext() PURE; + virtual ProcessContext* processContext() PURE; }; class ListenerFactoryContext : public virtual FactoryContext { diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index d87e72f84482..95f7b5abba61 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -196,7 +196,7 @@ class Instance { /** * @return the server-wide process context. */ - virtual ProcessContext& processContext() PURE; + virtual ProcessContext* processContext() PURE; /** * @return ThreadLocal::Instance& the thread local storage engine for the server. This is used to diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index bbd350b325f1..58262c7da3a7 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -94,7 +94,7 @@ class ValidationInstance : Logger::Loggable, Stats::Store& stats() override { return stats_store_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - ProcessContext& processContext() override { NOT_IMPLEMENTED_GCOVR_EXCL_LINE; } + ProcessContext* processContext() override { return nullptr; } ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } TimeSource& timeSource() override { return api_->timeSource(); } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index b634a1bf6537..97186031b968 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -324,7 +324,7 @@ class ListenerImpl : public Network::ListenerConfig, ServerLifecycleNotifier& lifecycleNotifier() override { return parent_.server_.lifecycleNotifier(); } - ProcessContext& processContext() override { return parent_.server_.processContext(); } + ProcessContext* processContext() override { return parent_.server_.processContext(); } // Network::DrainDecision bool drainClose() const override; diff --git a/source/server/server.h b/source/server/server.h index 500f743f98a6..7f7db01c34e2 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -194,7 +194,7 @@ class InstanceImpl : Logger::Loggable, Stats::Store& stats() override { return stats_store_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - ProcessContext& processContext() override { return *process_context_; } + ProcessContext* processContext() override { return process_context_.get(); } ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } TimeSource& timeSource() override { return time_source_; } diff --git a/test/integration/filters/process_context_filter.cc b/test/integration/filters/process_context_filter.cc index 6765042b2c2a..e3394dff0056 100644 --- a/test/integration/filters/process_context_filter.cc +++ b/test/integration/filters/process_context_filter.cc @@ -44,7 +44,7 @@ class ProcessContextFilterConfig : public Extensions::HttpFilters::Common::Empty Server::Configuration::FactoryContext& factory_context) override { return [&factory_context](Http::FilterChainFactoryCallbacks& callbacks) { callbacks.addStreamFilter( - std::make_shared(factory_context.processContext())); + std::make_shared(*factory_context.processContext())); }; } }; diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index de7d678c383b..003190088059 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -382,7 +382,7 @@ class MockInstance : public Instance { MOCK_METHOD0(stats, Stats::Store&()); MOCK_METHOD0(grpcContext, Grpc::Context&()); MOCK_METHOD0(httpContext, Http::Context&()); - MOCK_METHOD0(processContext, ProcessContext&()); + MOCK_METHOD0(processContext, ProcessContext*()); MOCK_METHOD0(threadLocal, ThreadLocal::Instance&()); MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_CONST_METHOD0(statsFlushInterval, std::chrono::milliseconds()); @@ -470,7 +470,7 @@ class MockFactoryContext : public virtual FactoryContext { Event::TestTimeSystem& timeSystem() { return time_system_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - MOCK_METHOD0(processContext, ProcessContext&()); + MOCK_METHOD0(processContext, ProcessContext*()); MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); MOCK_METHOD0(api, Api::Api&()); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 4f0cf9ecbc60..49d71cc4eb33 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -839,8 +839,8 @@ TEST_P(ServerInstanceImplTest, WithProcessContext) { EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); - ProcessContext& context = server_->processContext(); - auto& object_from_context = dynamic_cast(context.get()); + ProcessContext* context = server_->processContext(); + auto& object_from_context = dynamic_cast(context->get()); EXPECT_EQ(&object_from_context, &object); EXPECT_TRUE(object_from_context.boolean_flag_); From f3ef28d3c3cf51d2a49fee2a5cff09e6e5c35608 Mon Sep 17 00:00:00 2001 From: Elisha Ziskind Date: Mon, 16 Sep 2019 09:41:37 -0400 Subject: [PATCH 2/3] Use absl::optional> Signed-off-by: Elisha Ziskind --- include/envoy/server/filter_config.h | 6 +++--- include/envoy/server/instance.h | 2 +- source/server/config_validation/server.h | 4 +++- source/server/listener_manager_impl.h | 4 +++- source/server/server.h | 4 +++- test/mocks/server/mocks.h | 4 ++-- test/server/server_test.cc | 2 +- 7 files changed, 16 insertions(+), 10 deletions(-) diff --git a/include/envoy/server/filter_config.h b/include/envoy/server/filter_config.h index 2739f2057f52..a6286ef4640b 100644 --- a/include/envoy/server/filter_config.h +++ b/include/envoy/server/filter_config.h @@ -180,10 +180,10 @@ class FactoryContext : public virtual CommonFactoryContext { virtual Grpc::Context& grpcContext() PURE; /** - * @return ProcessContext* a pointer to the process context. Will be null when running in - * validation mode. + * @return absl::optional> an optional reference to the + * process context. Will be unset when running in validation mode. */ - virtual ProcessContext* processContext() PURE; + virtual absl::optional> processContext() PURE; }; class ListenerFactoryContext : public virtual FactoryContext { diff --git a/include/envoy/server/instance.h b/include/envoy/server/instance.h index 95f7b5abba61..d440c8cf4c3d 100644 --- a/include/envoy/server/instance.h +++ b/include/envoy/server/instance.h @@ -196,7 +196,7 @@ class Instance { /** * @return the server-wide process context. */ - virtual ProcessContext* processContext() PURE; + virtual absl::optional> processContext() PURE; /** * @return ThreadLocal::Instance& the thread local storage engine for the server. This is used to diff --git a/source/server/config_validation/server.h b/source/server/config_validation/server.h index 58262c7da3a7..8a59c4a046ff 100644 --- a/source/server/config_validation/server.h +++ b/source/server/config_validation/server.h @@ -94,7 +94,9 @@ class ValidationInstance : Logger::Loggable, Stats::Store& stats() override { return stats_store_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - ProcessContext* processContext() override { return nullptr; } + absl::optional> processContext() override { + return absl::nullopt; + } ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } TimeSource& timeSource() override { return api_->timeSource(); } diff --git a/source/server/listener_manager_impl.h b/source/server/listener_manager_impl.h index 97186031b968..35bd7ddfee28 100644 --- a/source/server/listener_manager_impl.h +++ b/source/server/listener_manager_impl.h @@ -324,7 +324,9 @@ class ListenerImpl : public Network::ListenerConfig, ServerLifecycleNotifier& lifecycleNotifier() override { return parent_.server_.lifecycleNotifier(); } - ProcessContext* processContext() override { return parent_.server_.processContext(); } + absl::optional> processContext() override { + return parent_.server_.processContext(); + } // Network::DrainDecision bool drainClose() const override; diff --git a/source/server/server.h b/source/server/server.h index 7f7db01c34e2..006b39d99cbb 100644 --- a/source/server/server.h +++ b/source/server/server.h @@ -194,7 +194,9 @@ class InstanceImpl : Logger::Loggable, Stats::Store& stats() override { return stats_store_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - ProcessContext* processContext() override { return process_context_.get(); } + absl::optional> processContext() override { + return *process_context_; + } ThreadLocal::Instance& threadLocal() override { return thread_local_; } const LocalInfo::LocalInfo& localInfo() override { return *local_info_; } TimeSource& timeSource() override { return time_source_; } diff --git a/test/mocks/server/mocks.h b/test/mocks/server/mocks.h index 003190088059..b8398a630b50 100644 --- a/test/mocks/server/mocks.h +++ b/test/mocks/server/mocks.h @@ -382,7 +382,7 @@ class MockInstance : public Instance { MOCK_METHOD0(stats, Stats::Store&()); MOCK_METHOD0(grpcContext, Grpc::Context&()); MOCK_METHOD0(httpContext, Http::Context&()); - MOCK_METHOD0(processContext, ProcessContext*()); + MOCK_METHOD0(processContext, absl::optional>()); MOCK_METHOD0(threadLocal, ThreadLocal::Instance&()); MOCK_METHOD0(localInfo, const LocalInfo::LocalInfo&()); MOCK_CONST_METHOD0(statsFlushInterval, std::chrono::milliseconds()); @@ -470,7 +470,7 @@ class MockFactoryContext : public virtual FactoryContext { Event::TestTimeSystem& timeSystem() { return time_system_; } Grpc::Context& grpcContext() override { return grpc_context_; } Http::Context& httpContext() override { return http_context_; } - MOCK_METHOD0(processContext, ProcessContext*()); + MOCK_METHOD0(processContext, absl::optional>()); MOCK_METHOD0(messageValidationVisitor, ProtobufMessage::ValidationVisitor&()); MOCK_METHOD0(api, Api::Api&()); diff --git a/test/server/server_test.cc b/test/server/server_test.cc index 49d71cc4eb33..a43e309318f9 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -839,7 +839,7 @@ TEST_P(ServerInstanceImplTest, WithProcessContext) { EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); - ProcessContext* context = server_->processContext(); + auto context = server_->processContext(); auto& object_from_context = dynamic_cast(context->get()); EXPECT_EQ(&object_from_context, &object); EXPECT_TRUE(object_from_context.boolean_flag_); From cb816b42d3914806141977a897d99ef73d0f7b1f Mon Sep 17 00:00:00 2001 From: Elisha Ziskind Date: Mon, 16 Sep 2019 10:17:53 -0400 Subject: [PATCH 3/3] Fix test Signed-off-by: Elisha Ziskind --- test/server/server_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/server/server_test.cc b/test/server/server_test.cc index a43e309318f9..24611a222bb9 100644 --- a/test/server/server_test.cc +++ b/test/server/server_test.cc @@ -840,7 +840,7 @@ TEST_P(ServerInstanceImplTest, WithProcessContext) { EXPECT_NO_THROW(initialize("test/server/empty_bootstrap.yaml")); auto context = server_->processContext(); - auto& object_from_context = dynamic_cast(context->get()); + auto& object_from_context = dynamic_cast(context->get().get()); EXPECT_EQ(&object_from_context, &object); EXPECT_TRUE(object_from_context.boolean_flag_);