From 3ece34d45c2b6e0d98d9546b1367054c90a523a6 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 31 Oct 2018 15:29:26 -0700 Subject: Let us clean a few things before getting started --- include/grpcpp/impl/codegen/call_op_set.h | 55 +++++++++++----------- include/grpcpp/impl/codegen/interceptor.h | 7 ++- include/grpcpp/impl/codegen/interceptor_common.h | 59 +++++++----------------- src/cpp/client/client_context.cc | 4 ++ 4 files changed, 54 insertions(+), 71 deletions(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 785688e67f..826a5972a0 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -214,11 +214,10 @@ class CallNoOp { void AddOp(grpc_op* ops, size_t* nops) {} void FinishOp(bool* status) {} void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) {} void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { - } + InterceptorBatchMethodsImpl* interceptor_methods) {} + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) {} }; class CallOpSendInitialMetadata { @@ -265,7 +264,7 @@ class CallOpSendInitialMetadata { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!send_) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA); @@ -273,9 +272,9 @@ class CallOpSendInitialMetadata { } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) {} - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; } @@ -318,7 +317,7 @@ class CallOpSendMessage { void FinishOp(bool* status) { send_buf_.Clear(); } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!send_buf_.Valid()) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE); @@ -326,9 +325,9 @@ class CallOpSendMessage { } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) {} - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; } @@ -406,17 +405,17 @@ class CallOpRecvMessage { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { interceptor_methods->SetRecvMessage(message_); } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!got_message) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::POST_RECV_MESSAGE); } - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; if (message_ == nullptr) return; interceptor_methods->AddInterceptionHookPoint( @@ -501,17 +500,17 @@ class CallOpGenericRecvMessage { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { interceptor_methods->SetRecvMessage(message_); } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!got_message) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::POST_RECV_MESSAGE); } - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; if (!deserialize_) return; interceptor_methods->AddInterceptionHookPoint( @@ -543,16 +542,16 @@ class CallOpClientSendClose { void FinishOp(bool* status) { send_ = false; } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!send_) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_CLOSE); } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) {} - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; } @@ -600,7 +599,7 @@ class CallOpServerSendStatus { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (!send_status_available_) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_STATUS); @@ -610,9 +609,9 @@ class CallOpServerSendStatus { } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) {} + InterceptorBatchMethodsImpl* interceptor_methods) {} - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; } @@ -652,19 +651,19 @@ class CallOpRecvInitialMetadata { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { interceptor_methods->SetRecvInitialMetadata(metadata_map_); } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (metadata_map_ == nullptr) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::POST_RECV_INITIAL_METADATA); metadata_map_ = nullptr; } - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; if (metadata_map_ == nullptr) return; interceptor_methods->AddInterceptionHookPoint( @@ -720,20 +719,20 @@ class CallOpClientRecvStatus { } void SetInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { interceptor_methods->SetRecvStatus(recv_status_); interceptor_methods->SetRecvTrailingMetadata(metadata_map_); } void SetFinishInterceptionHookPoint( - InternalInterceptorBatchMethods* interceptor_methods) { + InterceptorBatchMethodsImpl* interceptor_methods) { if (recv_status_ == nullptr) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::POST_RECV_STATUS); recv_status_ = nullptr; } - void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { + void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { hijacked_ = true; if (recv_status_ == nullptr) return; interceptor_methods->AddInterceptionHookPoint( diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 15cab711e5..4fb322aa8e 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -56,6 +56,9 @@ enum class InterceptionHookPoints { POST_RECV_MESSAGE, POST_RECV_STATUS /* client only */, POST_RECV_CLOSE /* server only */, + /* This is a special hook point available to both clients and servers. It is + illegal for an interceptor to block/delay this operation */ + PRE_SEND_CANCEL, NUM_INTERCEPTION_HOOKS }; @@ -66,7 +69,9 @@ class InterceptorBatchMethods { // of type \a type virtual bool QueryInterceptionHookPoint(InterceptionHookPoints type) = 0; // Calling this will signal that the interceptor is done intercepting the - // current batch of the RPC + // current batch of the RPC. + // Proceed is a no-op if the batch contains PRE_SEND_CANCEL. Simply returning + // from the Intercept method does the job of continuing the RPC. virtual void Proceed() = 0; // Calling this indicates that the interceptor has hijacked the RPC (only // valid if the batch contains send_initial_metadata on the client side) diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index cf564977f6..d47e28d615 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -19,7 +19,12 @@ #ifndef GRPCPP_IMPL_CODEGEN_INTERCEPTOR_COMMON_H #define GRPCPP_IMPL_CODEGEN_INTERCEPTOR_COMMON_H +#include + +#include +#include #include +#include #include #include @@ -27,37 +32,8 @@ namespace grpc { namespace internal { -/// Internal methods for setting the state -class InternalInterceptorBatchMethods +class InterceptorBatchMethodsImpl : public experimental::InterceptorBatchMethods { - public: - virtual ~InternalInterceptorBatchMethods() {} - - virtual void AddInterceptionHookPoint( - experimental::InterceptionHookPoints type) = 0; - - virtual void SetSendMessage(ByteBuffer* buf) = 0; - - virtual void SetSendInitialMetadata( - std::multimap* metadata) = 0; - - virtual void SetSendStatus(grpc_status_code* code, - grpc::string* error_details, - grpc::string* error_message) = 0; - - virtual void SetSendTrailingMetadata( - std::multimap* metadata) = 0; - - virtual void SetRecvMessage(void* message) = 0; - - virtual void SetRecvInitialMetadata(MetadataMap* map) = 0; - - virtual void SetRecvStatus(Status* status) = 0; - - virtual void SetRecvTrailingMetadata(MetadataMap* map) = 0; -}; - -class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { public: InterceptorBatchMethodsImpl() { for (auto i = static_cast(0); @@ -75,7 +51,7 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { return hooks_[static_cast(type)]; } - void Proceed() override { /* fill this */ + void Proceed() override { if (call_->client_rpc_info() != nullptr) { return ProceedClient(); } @@ -98,8 +74,7 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { rpc_info->RunInterceptor(this, current_interceptor_index_); } - void AddInterceptionHookPoint( - experimental::InterceptionHookPoints type) override { + void AddInterceptionHookPoint(experimental::InterceptionHookPoints type) { hooks_[static_cast(type)] = true; } @@ -139,38 +114,38 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { return recv_trailing_metadata_->map(); } - void SetSendMessage(ByteBuffer* buf) override { send_message_ = buf; } + void SetSendMessage(ByteBuffer* buf) { send_message_ = buf; } void SetSendInitialMetadata( - std::multimap* metadata) override { + std::multimap* metadata) { send_initial_metadata_ = metadata; } void SetSendStatus(grpc_status_code* code, grpc::string* error_details, - grpc::string* error_message) override { + grpc::string* error_message) { code_ = code; error_details_ = error_details; error_message_ = error_message; } void SetSendTrailingMetadata( - std::multimap* metadata) override { + std::multimap* metadata) { send_trailing_metadata_ = metadata; } - void SetRecvMessage(void* message) override { recv_message_ = message; } + void SetRecvMessage(void* message) { recv_message_ = message; } - void SetRecvInitialMetadata(MetadataMap* map) override { + void SetRecvInitialMetadata(MetadataMap* map) { recv_initial_metadata_ = map; } - void SetRecvStatus(Status* status) override { recv_status_ = status; } + void SetRecvStatus(Status* status) { recv_status_ = status; } - void SetRecvTrailingMetadata(MetadataMap* map) override { + void SetRecvTrailingMetadata(MetadataMap* map) { recv_trailing_metadata_ = map; } - std::unique_ptr GetInterceptedChannel() override { + std::unique_ptr GetInterceptedChannel() { auto* info = call_->client_rpc_info(); if (info == nullptr) { return std::unique_ptr(nullptr); diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index 07a04e4268..8cdcddca87 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -24,6 +24,7 @@ #include #include +#include #include #include #include @@ -110,6 +111,9 @@ void ClientContext::set_compression_algorithm( void ClientContext::TryCancel() { std::unique_lock lock(mu_); if (call_) { + // for(size_t i = 0; i < rpc_info_.interceptors_.size(); i++) { + // rpc_info_.RunInterceptor(, 0); + //} grpc_call_cancel(call_, nullptr); } else { call_canceled_ = true; -- cgit v1.2.3 From fd88dcaf5537839a30e4f958a01c311f3e876c26 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 11:58:46 -0700 Subject: Add cancellation notification --- include/grpcpp/impl/codegen/interceptor_common.h | 99 ++++++++++++++++++++++++ src/cpp/client/client_context.cc | 7 +- src/cpp/server/server_context.cc | 6 ++ 3 files changed, 109 insertions(+), 3 deletions(-) diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index d47e28d615..f520e7905a 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -352,6 +352,105 @@ class InterceptorBatchMethodsImpl MetadataMap* recv_trailing_metadata_ = nullptr; }; +// A special implementation of InterceptorBatchMethods to send a Cancel +// notification down the interceptor stack +class CancelInterceptorBatchMethods + : public experimental::InterceptorBatchMethods { + public: + bool QueryInterceptionHookPoint( + experimental::InterceptionHookPoints type) override { + if (type == experimental::InterceptionHookPoints::PRE_SEND_CANCEL) { + return true; + } else { + return false; + } + } + + void Proceed() override { + // This is a no-op. For actual continuation of the RPC simply needs to + // return from the Intercept method + } + + void Hijack() override { + // Only the client can hijack when sending down initial metadata + GPR_CODEGEN_ASSERT(false && + "It is illegal to call Hijack on a method which has a " + "Cancel notification"); + } + + ByteBuffer* GetSendMessage() override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetSendMessage on a method which " + "has a Cancel notification"); + return nullptr; + } + + std::multimap* GetSendInitialMetadata() override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetSendInitialMetadata on a " + "method which has a Cancel notification"); + return nullptr; + } + + Status GetSendStatus() override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetSendStatus on a method which " + "has a Cancel notification"); + return Status(); + } + + void ModifySendStatus(const Status& status) override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call ModifySendStatus on a method " + "which has a Cancel notification"); + return; + } + + std::multimap* GetSendTrailingMetadata() + override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetSendTrailingMetadata on a " + "method which has a Cancel notification"); + return nullptr; + } + + void* GetRecvMessage() override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetRecvMessage on a method which " + "has a Cancel notification"); + return nullptr; + } + + std::multimap* GetRecvInitialMetadata() + override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetRecvInitialMetadata on a " + "method which has a Cancel notification"); + return nullptr; + } + + Status* GetRecvStatus() override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetRecvStatus on a method which " + "has a Cancel notification"); + return nullptr; + } + + std::multimap* GetRecvTrailingMetadata() + override { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetRecvTrailingMetadata on a " + "method which has a Cancel notification"); + return nullptr; + } + + std::unique_ptr GetInterceptedChannel() { + GPR_CODEGEN_ASSERT(false && + "It is illegal to call GetInterceptedChannel on a " + "method which has a Cancel notification"); + return std::unique_ptr(nullptr); + } +}; } // namespace internal } // namespace grpc diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index 8cdcddca87..d5eb029a24 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -111,9 +111,10 @@ void ClientContext::set_compression_algorithm( void ClientContext::TryCancel() { std::unique_lock lock(mu_); if (call_) { - // for(size_t i = 0; i < rpc_info_.interceptors_.size(); i++) { - // rpc_info_.RunInterceptor(, 0); - //} + internal::CancelInterceptorBatchMethods cancel_methods; + for (size_t i = 0; i < rpc_info_.interceptors_.size(); i++) { + rpc_info_.RunInterceptor(&cancel_methods, i); + } grpc_call_cancel(call_, nullptr); } else { call_canceled_ = true; diff --git a/src/cpp/server/server_context.cc b/src/cpp/server/server_context.cc index 995e787785..e61fec9966 100644 --- a/src/cpp/server/server_context.cc +++ b/src/cpp/server/server_context.cc @@ -275,6 +275,12 @@ void ServerContext::AddTrailingMetadata(const grpc::string& key, } void ServerContext::TryCancel() const { + internal::CancelInterceptorBatchMethods cancel_methods; + if (rpc_info_) { + for (size_t i = 0; i < rpc_info_->interceptors_.size(); i++) { + rpc_info_->RunInterceptor(&cancel_methods, i); + } + } grpc_call_error err = grpc_call_cancel_with_status( call_, GRPC_STATUS_CANCELLED, "Cancelled on the server side", nullptr); if (err != GRPC_CALL_OK) { -- cgit v1.2.3 From d736b1d3090ed6f4ca570066de2788e8f8164431 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 12:30:40 -0700 Subject: Refactor tests slightly --- CMakeLists.txt | 2 + Makefile | 6 + build.yaml | 2 + test/cpp/end2end/BUILD | 1 + .../end2end/client_interceptors_end2end_test.cc | 45 ------ test/cpp/end2end/interceptors_util.cc | 134 +++++++++++++++++ test/cpp/end2end/interceptors_util.h | 164 ++++++++------------- .../end2end/server_interceptors_end2end_test.cc | 45 ------ tools/run_tests/generated/sources_and_headers.json | 2 + 9 files changed, 209 insertions(+), 192 deletions(-) create mode 100644 test/cpp/end2end/interceptors_util.cc diff --git a/CMakeLists.txt b/CMakeLists.txt index 700fa48abc..e5ae495c4e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12434,6 +12434,7 @@ if (gRPC_BUILD_TESTS) add_executable(client_interceptors_end2end_test test/cpp/end2end/client_interceptors_end2end_test.cc + test/cpp/end2end/interceptors_util.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc ) @@ -15344,6 +15345,7 @@ endif (gRPC_BUILD_TESTS) if (gRPC_BUILD_TESTS) add_executable(server_interceptors_end2end_test + test/cpp/end2end/interceptors_util.cc test/cpp/end2end/server_interceptors_end2end_test.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc diff --git a/Makefile b/Makefile index 19c518427f..4131a44480 100644 --- a/Makefile +++ b/Makefile @@ -17316,6 +17316,7 @@ endif CLIENT_INTERCEPTORS_END2END_TEST_SRC = \ test/cpp/end2end/client_interceptors_end2end_test.cc \ + test/cpp/end2end/interceptors_util.cc \ CLIENT_INTERCEPTORS_END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CLIENT_INTERCEPTORS_END2END_TEST_SRC)))) ifeq ($(NO_SECURE),true) @@ -17348,6 +17349,8 @@ endif $(OBJDIR)/$(CONFIG)/test/cpp/end2end/client_interceptors_end2end_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(OBJDIR)/$(CONFIG)/test/cpp/end2end/interceptors_util.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + deps_client_interceptors_end2end_test: $(CLIENT_INTERCEPTORS_END2END_TEST_OBJS:.o=.dep) ifneq ($(NO_SECURE),true) @@ -20143,6 +20146,7 @@ endif SERVER_INTERCEPTORS_END2END_TEST_SRC = \ + test/cpp/end2end/interceptors_util.cc \ test/cpp/end2end/server_interceptors_end2end_test.cc \ SERVER_INTERCEPTORS_END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(SERVER_INTERCEPTORS_END2END_TEST_SRC)))) @@ -20174,6 +20178,8 @@ endif endif +$(OBJDIR)/$(CONFIG)/test/cpp/end2end/interceptors_util.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(OBJDIR)/$(CONFIG)/test/cpp/end2end/server_interceptors_end2end_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a deps_server_interceptors_end2end_test: $(SERVER_INTERCEPTORS_END2END_TEST_OBJS:.o=.dep) diff --git a/build.yaml b/build.yaml index e7e92d280d..99646f2b8b 100644 --- a/build.yaml +++ b/build.yaml @@ -4537,6 +4537,7 @@ targets: - test/cpp/end2end/interceptors_util.h src: - test/cpp/end2end/client_interceptors_end2end_test.cc + - test/cpp/end2end/interceptors_util.cc deps: - grpc++_test_util - grpc_test_util @@ -5464,6 +5465,7 @@ targets: headers: - test/cpp/end2end/interceptors_util.h src: + - test/cpp/end2end/interceptors_util.cc - test/cpp/end2end/server_interceptors_end2end_test.cc deps: - grpc++_test_util diff --git a/test/cpp/end2end/BUILD b/test/cpp/end2end/BUILD index 235249e8bf..f83ce0a586 100644 --- a/test/cpp/end2end/BUILD +++ b/test/cpp/end2end/BUILD @@ -38,6 +38,7 @@ grpc_cc_library( grpc_cc_library( name = "interceptors_util", testonly = True, + srcs = ["interceptors_util.cc"], hdrs = ["interceptors_util.h"], external_deps = [ "gtest", diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index e8ffd46344..34859e9639 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -81,51 +81,6 @@ class ClientInterceptorsEnd2endTest : public ::testing::Test { std::unique_ptr server_; }; -/* This interceptor does nothing. Just keeps a global count on the number of - * times it was invoked. */ -class DummyInterceptor : public experimental::Interceptor { - public: - DummyInterceptor(experimental::ClientRpcInfo* info) {} - - virtual void Intercept(experimental::InterceptorBatchMethods* methods) { - if (methods->QueryInterceptionHookPoint( - experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { - num_times_run_++; - } else if (methods->QueryInterceptionHookPoint( - experimental::InterceptionHookPoints:: - POST_RECV_INITIAL_METADATA)) { - num_times_run_reverse_++; - } - methods->Proceed(); - } - - static void Reset() { - num_times_run_.store(0); - num_times_run_reverse_.store(0); - } - - static int GetNumTimesRun() { - EXPECT_EQ(num_times_run_.load(), num_times_run_reverse_.load()); - return num_times_run_.load(); - } - - private: - static std::atomic num_times_run_; - static std::atomic num_times_run_reverse_; -}; - -std::atomic DummyInterceptor::num_times_run_; -std::atomic DummyInterceptor::num_times_run_reverse_; - -class DummyInterceptorFactory - : public experimental::ClientInterceptorFactoryInterface { - public: - virtual experimental::Interceptor* CreateClientInterceptor( - experimental::ClientRpcInfo* info) override { - return new DummyInterceptor(info); - } -}; - /* Hijacks Echo RPC and fills in the expected values */ class HijackingInterceptor : public experimental::Interceptor { public: diff --git a/test/cpp/end2end/interceptors_util.cc b/test/cpp/end2end/interceptors_util.cc new file mode 100644 index 0000000000..7dad7ef567 --- /dev/null +++ b/test/cpp/end2end/interceptors_util.cc @@ -0,0 +1,134 @@ +/* + * + * Copyright 2018 gRPC authors. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + * + */ + +#include "test/cpp/end2end/interceptors_util.h" + +namespace grpc { +namespace testing { + +std::atomic DummyInterceptor::num_times_run_; +std::atomic DummyInterceptor::num_times_run_reverse_; + +void MakeCall(const std::shared_ptr& channel) { + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + req.mutable_param()->set_echo_metadata(true); + ctx.AddMetadata("testkey", "testvalue"); + req.set_message("Hello"); + EchoResponse resp; + Status s = stub->Echo(&ctx, req, &resp); + EXPECT_EQ(s.ok(), true); + EXPECT_EQ(resp.message(), "Hello"); +} + +void MakeClientStreamingCall(const std::shared_ptr& channel) { + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + req.mutable_param()->set_echo_metadata(true); + ctx.AddMetadata("testkey", "testvalue"); + req.set_message("Hello"); + EchoResponse resp; + string expected_resp = ""; + auto writer = stub->RequestStream(&ctx, &resp); + for (int i = 0; i < 10; i++) { + writer->Write(req); + expected_resp += "Hello"; + } + writer->WritesDone(); + Status s = writer->Finish(); + EXPECT_EQ(s.ok(), true); + EXPECT_EQ(resp.message(), expected_resp); +} + +void MakeServerStreamingCall(const std::shared_ptr& channel) { + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + req.mutable_param()->set_echo_metadata(true); + ctx.AddMetadata("testkey", "testvalue"); + req.set_message("Hello"); + EchoResponse resp; + string expected_resp = ""; + auto reader = stub->ResponseStream(&ctx, req); + int count = 0; + while (reader->Read(&resp)) { + EXPECT_EQ(resp.message(), "Hello"); + count++; + } + ASSERT_EQ(count, 10); + Status s = reader->Finish(); + EXPECT_EQ(s.ok(), true); +} + +void MakeBidiStreamingCall(const std::shared_ptr& channel) { + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + EchoResponse resp; + ctx.AddMetadata("testkey", "testvalue"); + auto stream = stub->BidiStream(&ctx); + for (auto i = 0; i < 10; i++) { + req.set_message("Hello" + std::to_string(i)); + stream->Write(req); + stream->Read(&resp); + EXPECT_EQ(req.message(), resp.message()); + } + ASSERT_TRUE(stream->WritesDone()); + Status s = stream->Finish(); + EXPECT_EQ(s.ok(), true); +} + +void MakeCallbackCall(const std::shared_ptr& channel) { + auto stub = grpc::testing::EchoTestService::NewStub(channel); + ClientContext ctx; + EchoRequest req; + std::mutex mu; + std::condition_variable cv; + bool done = false; + req.mutable_param()->set_echo_metadata(true); + ctx.AddMetadata("testkey", "testvalue"); + req.set_message("Hello"); + EchoResponse resp; + stub->experimental_async()->Echo(&ctx, &req, &resp, + [&resp, &mu, &done, &cv](Status s) { + // gpr_log(GPR_ERROR, "got the callback"); + EXPECT_EQ(s.ok(), true); + EXPECT_EQ(resp.message(), "Hello"); + std::lock_guard l(mu); + done = true; + cv.notify_one(); + }); + std::unique_lock l(mu); + while (!done) { + cv.wait(l); + } +} + +bool CheckMetadata(const std::multimap& map, + const string& key, const string& value) { + for (const auto& pair : map) { + if (pair.first.starts_with(key) && pair.second.starts_with(value)) { + return true; + } + } + return false; +} +} // namespace testing +} // namespace grpc diff --git a/test/cpp/end2end/interceptors_util.h b/test/cpp/end2end/interceptors_util.h index 5f0aa37dc0..210c6b98ed 100644 --- a/test/cpp/end2end/interceptors_util.h +++ b/test/cpp/end2end/interceptors_util.h @@ -16,6 +16,10 @@ * */ +#include + +#include + #include "src/proto/grpc/testing/echo.grpc.pb.h" #include "test/cpp/util/string_ref_helper.h" @@ -23,6 +27,54 @@ namespace grpc { namespace testing { +/* This interceptor does nothing. Just keeps a global count on the number of + * times it was invoked. */ +class DummyInterceptor : public experimental::Interceptor { + public: + DummyInterceptor() {} + + virtual void Intercept(experimental::InterceptorBatchMethods* methods) { + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { + num_times_run_++; + } else if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints:: + POST_RECV_INITIAL_METADATA)) { + num_times_run_reverse_++; + } + methods->Proceed(); + } + + static void Reset() { + num_times_run_.store(0); + num_times_run_reverse_.store(0); + } + + static int GetNumTimesRun() { + EXPECT_EQ(num_times_run_.load(), num_times_run_reverse_.load()); + return num_times_run_.load(); + } + + private: + static std::atomic num_times_run_; + static std::atomic num_times_run_reverse_; +}; + +class DummyInterceptorFactory + : public experimental::ClientInterceptorFactoryInterface, + public experimental::ServerInterceptorFactoryInterface { + public: + virtual experimental::Interceptor* CreateClientInterceptor( + experimental::ClientRpcInfo* info) override { + return new DummyInterceptor(); + } + + virtual experimental::Interceptor* CreateServerInterceptor( + experimental::ServerRpcInfo* info) override { + return new DummyInterceptor(); + } +}; + class EchoTestServiceStreamingImpl : public EchoTestService::Service { public: ~EchoTestServiceStreamingImpl() override {} @@ -77,115 +129,23 @@ class EchoTestServiceStreamingImpl : public EchoTestService::Service { } }; -void MakeCall(const std::shared_ptr& channel) { - auto stub = grpc::testing::EchoTestService::NewStub(channel); - ClientContext ctx; - EchoRequest req; - req.mutable_param()->set_echo_metadata(true); - ctx.AddMetadata("testkey", "testvalue"); - req.set_message("Hello"); - EchoResponse resp; - Status s = stub->Echo(&ctx, req, &resp); - EXPECT_EQ(s.ok(), true); - EXPECT_EQ(resp.message(), "Hello"); -} +void MakeCall(const std::shared_ptr& channel); -void MakeClientStreamingCall(const std::shared_ptr& channel) { - auto stub = grpc::testing::EchoTestService::NewStub(channel); - ClientContext ctx; - EchoRequest req; - req.mutable_param()->set_echo_metadata(true); - ctx.AddMetadata("testkey", "testvalue"); - req.set_message("Hello"); - EchoResponse resp; - string expected_resp = ""; - auto writer = stub->RequestStream(&ctx, &resp); - for (int i = 0; i < 10; i++) { - writer->Write(req); - expected_resp += "Hello"; - } - writer->WritesDone(); - Status s = writer->Finish(); - EXPECT_EQ(s.ok(), true); - EXPECT_EQ(resp.message(), expected_resp); -} +void MakeClientStreamingCall(const std::shared_ptr& channel); -void MakeServerStreamingCall(const std::shared_ptr& channel) { - auto stub = grpc::testing::EchoTestService::NewStub(channel); - ClientContext ctx; - EchoRequest req; - req.mutable_param()->set_echo_metadata(true); - ctx.AddMetadata("testkey", "testvalue"); - req.set_message("Hello"); - EchoResponse resp; - string expected_resp = ""; - auto reader = stub->ResponseStream(&ctx, req); - int count = 0; - while (reader->Read(&resp)) { - EXPECT_EQ(resp.message(), "Hello"); - count++; - } - ASSERT_EQ(count, 10); - Status s = reader->Finish(); - EXPECT_EQ(s.ok(), true); -} +void MakeServerStreamingCall(const std::shared_ptr& channel); -void MakeBidiStreamingCall(const std::shared_ptr& channel) { - auto stub = grpc::testing::EchoTestService::NewStub(channel); - ClientContext ctx; - EchoRequest req; - EchoResponse resp; - ctx.AddMetadata("testkey", "testvalue"); - auto stream = stub->BidiStream(&ctx); - for (auto i = 0; i < 10; i++) { - req.set_message("Hello" + std::to_string(i)); - stream->Write(req); - stream->Read(&resp); - EXPECT_EQ(req.message(), resp.message()); - } - ASSERT_TRUE(stream->WritesDone()); - Status s = stream->Finish(); - EXPECT_EQ(s.ok(), true); -} +void MakeBidiStreamingCall(const std::shared_ptr& channel); -void MakeCallbackCall(const std::shared_ptr& channel) { - auto stub = grpc::testing::EchoTestService::NewStub(channel); - ClientContext ctx; - EchoRequest req; - std::mutex mu; - std::condition_variable cv; - bool done = false; - req.mutable_param()->set_echo_metadata(true); - ctx.AddMetadata("testkey", "testvalue"); - req.set_message("Hello"); - EchoResponse resp; - stub->experimental_async()->Echo(&ctx, &req, &resp, - [&resp, &mu, &done, &cv](Status s) { - // gpr_log(GPR_ERROR, "got the callback"); - EXPECT_EQ(s.ok(), true); - EXPECT_EQ(resp.message(), "Hello"); - std::lock_guard l(mu); - done = true; - cv.notify_one(); - }); - std::unique_lock l(mu); - while (!done) { - cv.wait(l); - } -} +void MakeCallbackCall(const std::shared_ptr& channel); bool CheckMetadata(const std::multimap& map, - const string& key, const string& value) { - for (const auto& pair : map) { - if (pair.first.starts_with(key) && pair.second.starts_with(value)) { - return true; - } - } - return false; -} + const string& key, const string& value); -void* tag(int i) { return (void*)static_cast(i); } -int detag(void* p) { return static_cast(reinterpret_cast(p)); } +inline void* tag(int i) { return (void*)static_cast(i); } +inline int detag(void* p) { + return static_cast(reinterpret_cast(p)); +} class Verifier { public: diff --git a/test/cpp/end2end/server_interceptors_end2end_test.cc b/test/cpp/end2end/server_interceptors_end2end_test.cc index e08a4493d3..4ae086ea76 100644 --- a/test/cpp/end2end/server_interceptors_end2end_test.cc +++ b/test/cpp/end2end/server_interceptors_end2end_test.cc @@ -42,51 +42,6 @@ namespace grpc { namespace testing { namespace { -/* This interceptor does nothing. Just keeps a global count on the number of - * times it was invoked. */ -class DummyInterceptor : public experimental::Interceptor { - public: - DummyInterceptor(experimental::ServerRpcInfo* info) {} - - virtual void Intercept(experimental::InterceptorBatchMethods* methods) { - if (methods->QueryInterceptionHookPoint( - experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { - num_times_run_++; - } else if (methods->QueryInterceptionHookPoint( - experimental::InterceptionHookPoints:: - POST_RECV_INITIAL_METADATA)) { - num_times_run_reverse_++; - } - methods->Proceed(); - } - - static void Reset() { - num_times_run_.store(0); - num_times_run_reverse_.store(0); - } - - static int GetNumTimesRun() { - EXPECT_EQ(num_times_run_.load(), num_times_run_reverse_.load()); - return num_times_run_.load(); - } - - private: - static std::atomic num_times_run_; - static std::atomic num_times_run_reverse_; -}; - -std::atomic DummyInterceptor::num_times_run_; -std::atomic DummyInterceptor::num_times_run_reverse_; - -class DummyInterceptorFactory - : public experimental::ServerInterceptorFactoryInterface { - public: - virtual experimental::Interceptor* CreateServerInterceptor( - experimental::ServerRpcInfo* info) override { - return new DummyInterceptor(info); - } -}; - class LoggingInterceptor : public experimental::Interceptor { public: LoggingInterceptor(experimental::ServerRpcInfo* info) { info_ = info; } diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 042856146d..415f370f4b 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3402,6 +3402,7 @@ "name": "client_interceptors_end2end_test", "src": [ "test/cpp/end2end/client_interceptors_end2end_test.cc", + "test/cpp/end2end/interceptors_util.cc", "test/cpp/end2end/interceptors_util.h" ], "third_party": false, @@ -4736,6 +4737,7 @@ "language": "c++", "name": "server_interceptors_end2end_test", "src": [ + "test/cpp/end2end/interceptors_util.cc", "test/cpp/end2end/interceptors_util.h", "test/cpp/end2end/server_interceptors_end2end_test.cc" ], -- cgit v1.2.3 From e2361a4751435f86b27ca7d4c690c38346c9ed63 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 14:06:43 -0700 Subject: Add server interceptors in end2end_test --- CMakeLists.txt | 1 + Makefile | 3 + build.yaml | 3 + test/cpp/end2end/BUILD | 1 + .../end2end/client_interceptors_end2end_test.cc | 76 +++++++++++----------- test/cpp/end2end/end2end_test.cc | 31 +++++++-- test/cpp/end2end/interceptors_util.cc | 1 + test/cpp/end2end/interceptors_util.h | 7 ++ tools/run_tests/generated/sources_and_headers.json | 8 ++- 9 files changed, 85 insertions(+), 46 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index e5ae495c4e..270fc13c11 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12888,6 +12888,7 @@ if (gRPC_BUILD_TESTS) add_executable(end2end_test test/cpp/end2end/end2end_test.cc + test/cpp/end2end/interceptor_util.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc ) diff --git a/Makefile b/Makefile index 4131a44480..6594f71e24 100644 --- a/Makefile +++ b/Makefile @@ -17754,6 +17754,7 @@ endif END2END_TEST_SRC = \ test/cpp/end2end/end2end_test.cc \ + test/cpp/end2end/interceptor_util.cc \ END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(END2END_TEST_SRC)))) ifeq ($(NO_SECURE),true) @@ -17786,6 +17787,8 @@ endif $(OBJDIR)/$(CONFIG)/test/cpp/end2end/end2end_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(OBJDIR)/$(CONFIG)/test/cpp/end2end/interceptor_util.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + deps_end2end_test: $(END2END_TEST_OBJS:.o=.dep) ifneq ($(NO_SECURE),true) diff --git a/build.yaml b/build.yaml index 99646f2b8b..bd30e54258 100644 --- a/build.yaml +++ b/build.yaml @@ -4664,8 +4664,11 @@ targets: cpu_cost: 0.5 build: test language: c++ + headers: + - test/cpp/end2end/interceptors_util.h src: - test/cpp/end2end/end2end_test.cc + - test/cpp/end2end/interceptor_util.cc deps: - grpc++_test_util - grpc_test_util diff --git a/test/cpp/end2end/BUILD b/test/cpp/end2end/BUILD index f83ce0a586..4e3d841db0 100644 --- a/test/cpp/end2end/BUILD +++ b/test/cpp/end2end/BUILD @@ -159,6 +159,7 @@ grpc_cc_library( "gtest", ], deps = [ + ":interceptors_util", ":test_service_impl", "//:gpr", "//:grpc", diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 34859e9639..c36417de37 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -43,44 +43,6 @@ namespace grpc { namespace testing { namespace { -class ClientInterceptorsStreamingEnd2endTest : public ::testing::Test { - protected: - ClientInterceptorsStreamingEnd2endTest() { - int port = grpc_pick_unused_port_or_die(); - - ServerBuilder builder; - server_address_ = "localhost:" + std::to_string(port); - builder.AddListeningPort(server_address_, InsecureServerCredentials()); - builder.RegisterService(&service_); - server_ = builder.BuildAndStart(); - } - - ~ClientInterceptorsStreamingEnd2endTest() { server_->Shutdown(); } - - std::string server_address_; - EchoTestServiceStreamingImpl service_; - std::unique_ptr server_; -}; - -class ClientInterceptorsEnd2endTest : public ::testing::Test { - protected: - ClientInterceptorsEnd2endTest() { - int port = grpc_pick_unused_port_or_die(); - - ServerBuilder builder; - server_address_ = "localhost:" + std::to_string(port); - builder.AddListeningPort(server_address_, InsecureServerCredentials()); - builder.RegisterService(&service_); - server_ = builder.BuildAndStart(); - } - - ~ClientInterceptorsEnd2endTest() { server_->Shutdown(); } - - std::string server_address_; - TestServiceImpl service_; - std::unique_ptr server_; -}; - /* Hijacks Echo RPC and fills in the expected values */ class HijackingInterceptor : public experimental::Interceptor { public: @@ -377,6 +339,25 @@ class LoggingInterceptorFactory } }; +class ClientInterceptorsEnd2endTest : public ::testing::Test { + protected: + ClientInterceptorsEnd2endTest() { + int port = grpc_pick_unused_port_or_die(); + + ServerBuilder builder; + server_address_ = "localhost:" + std::to_string(port); + builder.AddListeningPort(server_address_, InsecureServerCredentials()); + builder.RegisterService(&service_); + server_ = builder.BuildAndStart(); + } + + ~ClientInterceptorsEnd2endTest() { server_->Shutdown(); } + + std::string server_address_; + TestServiceImpl service_; + std::unique_ptr server_; +}; + TEST_F(ClientInterceptorsEnd2endTest, ClientInterceptorLoggingTest) { ChannelArguments args; DummyInterceptor::Reset(); @@ -493,6 +474,25 @@ TEST_F(ClientInterceptorsEnd2endTest, EXPECT_EQ(DummyInterceptor::GetNumTimesRun(), 20); } +class ClientInterceptorsStreamingEnd2endTest : public ::testing::Test { + protected: + ClientInterceptorsStreamingEnd2endTest() { + int port = grpc_pick_unused_port_or_die(); + + ServerBuilder builder; + server_address_ = "localhost:" + std::to_string(port); + builder.AddListeningPort(server_address_, InsecureServerCredentials()); + builder.RegisterService(&service_); + server_ = builder.BuildAndStart(); + } + + ~ClientInterceptorsStreamingEnd2endTest() { server_->Shutdown(); } + + std::string server_address_; + EchoTestServiceStreamingImpl service_; + std::unique_ptr server_; +}; + TEST_F(ClientInterceptorsStreamingEnd2endTest, ClientStreamingTest) { ChannelArguments args; DummyInterceptor::Reset(); diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index fc07681535..6fce4274a1 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -40,6 +40,7 @@ #include "src/proto/grpc/testing/echo.grpc.pb.h" #include "test/core/util/port.h" #include "test/core/util/test_config.h" +#include "test/cpp/end2end/interceptors_util.h" #include "test/cpp/end2end/test_service_impl.h" #include "test/cpp/util/string_ref_helper.h" #include "test/cpp/util/test_credentials_provider.h" @@ -179,7 +180,7 @@ class Proxy : public ::grpc::testing::EchoTestService::Service { } private: - std::unique_ptr< ::grpc::testing::EchoTestService::Stub> stub_; + std::unique_ptr<::grpc::testing::EchoTestService::Stub> stub_; }; class TestServiceImplDupPkg @@ -194,9 +195,14 @@ class TestServiceImplDupPkg class TestScenario { public: - TestScenario(bool proxy, bool inproc_stub, const grpc::string& creds_type) - : use_proxy(proxy), inproc(inproc_stub), credentials_type(creds_type) {} + TestScenario(bool interceptors, bool proxy, bool inproc_stub, + const grpc::string& creds_type) + : use_interceptors(interceptors), + use_proxy(proxy), + inproc(inproc_stub), + credentials_type(creds_type) {} void Log() const; + bool use_interceptors; bool use_proxy; bool inproc; const grpc::string credentials_type; @@ -260,6 +266,16 @@ class End2endTest : public ::testing::TestWithParam { if (GetParam().credentials_type != kInsecureCredentialsType) { server_creds->SetAuthMetadataProcessor(processor); } + if (GetParam().use_interceptors) { + std::vector< + std::unique_ptr> + creators; + // Add 20 dummy server interceptors + for (auto i = 0; i < 20; i++) { + creators.push_back(std::unique_ptr( + new DummyInterceptorFactory())); + } + } builder.AddListeningPort(server_address_.str(), server_creds); builder.RegisterService(&service_); builder.RegisterService("foo.test.youtube.com", &special_service_); @@ -1802,13 +1818,16 @@ std::vector CreateTestScenarios(bool use_proxy, } GPR_ASSERT(!credentials_types.empty()); for (const auto& cred : credentials_types) { - scenarios.emplace_back(false, false, cred); + scenarios.emplace_back(false, false, false, cred); + scenarios.emplace_back(true, false, false, cred); if (use_proxy) { - scenarios.emplace_back(true, false, cred); + scenarios.emplace_back(false, true, false, cred); + scenarios.emplace_back(true, true, false, cred); } } if (test_inproc && insec_ok()) { - scenarios.emplace_back(false, true, kInsecureCredentialsType); + scenarios.emplace_back(false, false, true, kInsecureCredentialsType); + scenarios.emplace_back(true, false, true, kInsecureCredentialsType); } return scenarios; } diff --git a/test/cpp/end2end/interceptors_util.cc b/test/cpp/end2end/interceptors_util.cc index 7dad7ef567..29fb49d3eb 100644 --- a/test/cpp/end2end/interceptors_util.cc +++ b/test/cpp/end2end/interceptors_util.cc @@ -23,6 +23,7 @@ namespace testing { std::atomic DummyInterceptor::num_times_run_; std::atomic DummyInterceptor::num_times_run_reverse_; +std::atomic DummyInterceptor::num_times_cancel_; void MakeCall(const std::shared_ptr& channel) { auto stub = grpc::testing::EchoTestService::NewStub(channel); diff --git a/test/cpp/end2end/interceptors_util.h b/test/cpp/end2end/interceptors_util.h index 210c6b98ed..f5b1d4a110 100644 --- a/test/cpp/end2end/interceptors_util.h +++ b/test/cpp/end2end/interceptors_util.h @@ -41,6 +41,9 @@ class DummyInterceptor : public experimental::Interceptor { experimental::InterceptionHookPoints:: POST_RECV_INITIAL_METADATA)) { num_times_run_reverse_++; + } else if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_CANCEL)) { + num_times_cancel_++; } methods->Proceed(); } @@ -48,6 +51,7 @@ class DummyInterceptor : public experimental::Interceptor { static void Reset() { num_times_run_.store(0); num_times_run_reverse_.store(0); + num_times_cancel_.store(0); } static int GetNumTimesRun() { @@ -55,9 +59,12 @@ class DummyInterceptor : public experimental::Interceptor { return num_times_run_.load(); } + static int GetNumTimesCancel() { return num_times_cancel_.load(); } + private: static std::atomic num_times_run_; static std::atomic num_times_run_reverse_; + static std::atomic num_times_cancel_; }; class DummyInterceptorFactory diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 415f370f4b..7491bd3e9c 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3601,12 +3601,16 @@ "grpc++_test_util", "grpc_test_util" ], - "headers": [], + "headers": [ + "test/cpp/end2end/interceptors_util.h" + ], "is_filegroup": false, "language": "c++", "name": "end2end_test", "src": [ - "test/cpp/end2end/end2end_test.cc" + "test/cpp/end2end/end2end_test.cc", + "test/cpp/end2end/interceptor_util.cc", + "test/cpp/end2end/interceptors_util.h" ], "third_party": false, "type": "target" -- cgit v1.2.3 From b732e9c4037e046354e959c67e789d9e3fcec5f1 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 16:14:40 -0700 Subject: Make client interceptors see notification even if Cancel was done before the RPC was issued. Also add tests --- CMakeLists.txt | 2 +- Makefile | 4 +- build.yaml | 2 +- include/grpcpp/impl/codegen/client_context.h | 2 + src/cpp/client/channel_cc.cc | 6 ++- src/cpp/client/client_context.cc | 15 +++++-- test/cpp/end2end/end2end_test.cc | 52 +++++++++++++++++++--- test/cpp/end2end/interceptors_util.cc | 16 +++++++ test/cpp/end2end/interceptors_util.h | 4 ++ tools/run_tests/generated/sources_and_headers.json | 2 +- 10 files changed, 89 insertions(+), 16 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 270fc13c11..bafc890121 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -12888,7 +12888,7 @@ if (gRPC_BUILD_TESTS) add_executable(end2end_test test/cpp/end2end/end2end_test.cc - test/cpp/end2end/interceptor_util.cc + test/cpp/end2end/interceptors_util.cc third_party/googletest/googletest/src/gtest-all.cc third_party/googletest/googlemock/src/gmock-all.cc ) diff --git a/Makefile b/Makefile index 6594f71e24..9cec533e20 100644 --- a/Makefile +++ b/Makefile @@ -17754,7 +17754,7 @@ endif END2END_TEST_SRC = \ test/cpp/end2end/end2end_test.cc \ - test/cpp/end2end/interceptor_util.cc \ + test/cpp/end2end/interceptors_util.cc \ END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(END2END_TEST_SRC)))) ifeq ($(NO_SECURE),true) @@ -17787,7 +17787,7 @@ endif $(OBJDIR)/$(CONFIG)/test/cpp/end2end/end2end_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a -$(OBJDIR)/$(CONFIG)/test/cpp/end2end/interceptor_util.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a +$(OBJDIR)/$(CONFIG)/test/cpp/end2end/interceptors_util.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a deps_end2end_test: $(END2END_TEST_OBJS:.o=.dep) diff --git a/build.yaml b/build.yaml index bd30e54258..ba95fcfdd9 100644 --- a/build.yaml +++ b/build.yaml @@ -4668,7 +4668,7 @@ targets: - test/cpp/end2end/interceptors_util.h src: - test/cpp/end2end/end2end_test.cc - - test/cpp/end2end/interceptor_util.cc + - test/cpp/end2end/interceptors_util.cc deps: - grpc++_test_util - grpc_test_util diff --git a/include/grpcpp/impl/codegen/client_context.h b/include/grpcpp/impl/codegen/client_context.h index f53b744dcf..75b955e760 100644 --- a/include/grpcpp/impl/codegen/client_context.h +++ b/include/grpcpp/impl/codegen/client_context.h @@ -426,6 +426,8 @@ class ClientContext { grpc::string authority() { return authority_; } + void SendCancelToInterceptors(); + bool initial_metadata_received_; bool wait_for_ready_; bool wait_for_ready_explicitly_set_; diff --git a/src/cpp/client/channel_cc.cc b/src/cpp/client/channel_cc.cc index 15e3ccb3c9..5e7ecf0ebf 100644 --- a/src/cpp/client/channel_cc.cc +++ b/src/cpp/client/channel_cc.cc @@ -147,10 +147,14 @@ internal::Call Channel::CreateCallInternal(const internal::RpcMethod& method, } } grpc_census_call_set_context(c_call, context->census_context()); - context->set_call(c_call, shared_from_this()); + // ClientRpcInfo should be set before call because set_call also checks + // whether the call has been cancelled, and if the call was cancelled, we + // should notify the interceptors too/ auto* info = context->set_client_rpc_info( method.name(), this, interceptor_creators_, interceptor_pos); + context->set_call(c_call, shared_from_this()); + return internal::Call(c_call, this, cq, info); } diff --git a/src/cpp/client/client_context.cc b/src/cpp/client/client_context.cc index d5eb029a24..50da75f09c 100644 --- a/src/cpp/client/client_context.cc +++ b/src/cpp/client/client_context.cc @@ -87,10 +87,13 @@ void ClientContext::set_call(grpc_call* call, call_ = call; channel_ = channel; if (creds_ && !creds_->ApplyToCall(call_)) { + // TODO(yashykt): should interceptors also see this status? + SendCancelToInterceptors(); grpc_call_cancel_with_status(call, GRPC_STATUS_CANCELLED, "Failed to set credentials to rpc.", nullptr); } if (call_canceled_) { + SendCancelToInterceptors(); grpc_call_cancel(call_, nullptr); } } @@ -111,16 +114,20 @@ void ClientContext::set_compression_algorithm( void ClientContext::TryCancel() { std::unique_lock lock(mu_); if (call_) { - internal::CancelInterceptorBatchMethods cancel_methods; - for (size_t i = 0; i < rpc_info_.interceptors_.size(); i++) { - rpc_info_.RunInterceptor(&cancel_methods, i); - } + SendCancelToInterceptors(); grpc_call_cancel(call_, nullptr); } else { call_canceled_ = true; } } +void ClientContext::SendCancelToInterceptors() { + internal::CancelInterceptorBatchMethods cancel_methods; + for (size_t i = 0; i < rpc_info_.interceptors_.size(); i++) { + rpc_info_.RunInterceptor(&cancel_methods, i); + } +} + grpc::string ClientContext::peer() const { grpc::string peer; if (call_) { diff --git a/test/cpp/end2end/end2end_test.cc b/test/cpp/end2end/end2end_test.cc index 6fce4274a1..4558437102 100644 --- a/test/cpp/end2end/end2end_test.cc +++ b/test/cpp/end2end/end2end_test.cc @@ -210,8 +210,9 @@ class TestScenario { static std::ostream& operator<<(std::ostream& out, const TestScenario& scenario) { - return out << "TestScenario{use_proxy=" - << (scenario.use_proxy ? "true" : "false") + return out << "TestScenario{use_interceptors=" + << (scenario.use_interceptors ? "true" : "false") + << ", use_proxy=" << (scenario.use_proxy ? "true" : "false") << ", inproc=" << (scenario.inproc ? "true" : "false") << ", credentials='" << scenario.credentials_type << "'}"; } @@ -275,6 +276,7 @@ class End2endTest : public ::testing::TestWithParam { creators.push_back(std::unique_ptr( new DummyInterceptorFactory())); } + builder.experimental().SetInterceptorCreators(std::move(creators)); } builder.AddListeningPort(server_address_.str(), server_creds); builder.RegisterService(&service_); @@ -308,10 +310,21 @@ class End2endTest : public ::testing::TestWithParam { args.SetString(GRPC_ARG_SECONDARY_USER_AGENT_STRING, "end2end_test"); if (!GetParam().inproc) { - channel_ = - CreateCustomChannel(server_address_.str(), channel_creds, args); + if (!GetParam().use_interceptors) { + channel_ = + CreateCustomChannel(server_address_.str(), channel_creds, args); + } else { + channel_ = CreateCustomChannelWithInterceptors( + server_address_.str(), channel_creds, args, + CreateDummyClientInterceptors()); + } } else { - channel_ = server_->InProcessChannel(args); + if (!GetParam().use_interceptors) { + channel_ = server_->InProcessChannel(args); + } else { + channel_ = server_->experimental().InProcessChannelWithInterceptors( + args, CreateDummyClientInterceptors()); + } } } @@ -336,6 +349,7 @@ class End2endTest : public ::testing::TestWithParam { } stub_ = grpc::testing::EchoTestService::NewStub(channel_); + DummyInterceptor::Reset(); } bool is_server_started_; @@ -392,6 +406,7 @@ class End2endServerTryCancelTest : public End2endTest { // NOTE: Do not call this function with server_try_cancel == DO_NOT_CANCEL. void TestRequestStreamServerCancel( ServerTryCancelRequestPhase server_try_cancel, int num_msgs_to_send) { + RestartServer(std::shared_ptr()); ResetStub(); EchoRequest request; EchoResponse response; @@ -448,6 +463,10 @@ class End2endServerTryCancelTest : public End2endTest { EXPECT_FALSE(s.ok()); EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); + // Make sure that the server interceptors were notified + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } // Helper for testing server-streaming RPCs which are cancelled on the server. @@ -465,6 +484,7 @@ class End2endServerTryCancelTest : public End2endTest { // NOTE: Do not call this function with server_try_cancel == DO_NOT_CANCEL. void TestResponseStreamServerCancel( ServerTryCancelRequestPhase server_try_cancel) { + RestartServer(std::shared_ptr()); ResetStub(); EchoRequest request; EchoResponse response; @@ -524,7 +544,10 @@ class End2endServerTryCancelTest : public End2endTest { } EXPECT_FALSE(s.ok()); - EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); + // Make sure that the server interceptors were notified + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } // Helper for testing bidirectional-streaming RPCs which are cancelled on the @@ -542,6 +565,7 @@ class End2endServerTryCancelTest : public End2endTest { // NOTE: Do not call this function with server_try_cancel == DO_NOT_CANCEL. void TestBidiStreamServerCancel(ServerTryCancelRequestPhase server_try_cancel, int num_messages) { + RestartServer(std::shared_ptr()); ResetStub(); EchoRequest request; EchoResponse response; @@ -608,6 +632,10 @@ class End2endServerTryCancelTest : public End2endTest { EXPECT_FALSE(s.ok()); EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); + // Make sure that the server interceptors were notified + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } }; @@ -1005,6 +1033,9 @@ TEST_P(End2endTest, CancelRpcBeforeStart) { Status s = stub_->Echo(&context, request, &response); EXPECT_EQ("", response.message()); EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } // Client cancels request stream after sending two messages @@ -1025,6 +1056,9 @@ TEST_P(End2endTest, ClientCancelsRequestStream) { EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); EXPECT_EQ(response.message(), ""); + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } // Client cancels server stream after sending some messages @@ -1057,6 +1091,9 @@ TEST_P(End2endTest, ClientCancelsResponseStream) { // The final status could be either of CANCELLED or OK depending on // who won the race. EXPECT_GE(grpc::StatusCode::CANCELLED, s.error_code()); + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } // Client cancels bidi stream after sending some messages @@ -1090,6 +1127,9 @@ TEST_P(End2endTest, ClientCancelsBidi) { Status s = stream->Finish(); EXPECT_EQ(grpc::StatusCode::CANCELLED, s.error_code()); + if (GetParam().use_interceptors) { + EXPECT_EQ(20, DummyInterceptor::GetNumTimesCancel()); + } } TEST_P(End2endTest, RpcMaxMessageSize) { diff --git a/test/cpp/end2end/interceptors_util.cc b/test/cpp/end2end/interceptors_util.cc index 29fb49d3eb..602d1695a3 100644 --- a/test/cpp/end2end/interceptors_util.cc +++ b/test/cpp/end2end/interceptors_util.cc @@ -131,5 +131,21 @@ bool CheckMetadata(const std::multimap& map, } return false; } + +std::unique_ptr>> +CreateDummyClientInterceptors() { + auto creators = std::unique_ptr>>( + new std::vector< + std::unique_ptr>()); + // Add 20 dummy interceptors before hijacking interceptor + for (auto i = 0; i < 20; i++) { + creators->push_back(std::unique_ptr( + new DummyInterceptorFactory())); + } + return creators; +} + } // namespace testing } // namespace grpc diff --git a/test/cpp/end2end/interceptors_util.h b/test/cpp/end2end/interceptors_util.h index f5b1d4a110..b4c4791fca 100644 --- a/test/cpp/end2end/interceptors_util.h +++ b/test/cpp/end2end/interceptors_util.h @@ -149,6 +149,10 @@ void MakeCallbackCall(const std::shared_ptr& channel); bool CheckMetadata(const std::multimap& map, const string& key, const string& value); +std::unique_ptr>> +CreateDummyClientInterceptors(); + inline void* tag(int i) { return (void*)static_cast(i); } inline int detag(void* p) { return static_cast(reinterpret_cast(p)); diff --git a/tools/run_tests/generated/sources_and_headers.json b/tools/run_tests/generated/sources_and_headers.json index 7491bd3e9c..ade814a844 100644 --- a/tools/run_tests/generated/sources_and_headers.json +++ b/tools/run_tests/generated/sources_and_headers.json @@ -3609,7 +3609,7 @@ "name": "end2end_test", "src": [ "test/cpp/end2end/end2end_test.cc", - "test/cpp/end2end/interceptor_util.cc", + "test/cpp/end2end/interceptors_util.cc", "test/cpp/end2end/interceptors_util.h" ], "third_party": false, -- cgit v1.2.3 From ddcadad5b5cffa6f8ddfc0d908ee117c34481ded Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 16:26:04 -0700 Subject: Add note on hijacking --- include/grpcpp/impl/codegen/interceptor.h | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 4fb322aa8e..19f6afcb72 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -56,8 +56,10 @@ enum class InterceptionHookPoints { POST_RECV_MESSAGE, POST_RECV_STATUS /* client only */, POST_RECV_CLOSE /* server only */, - /* This is a special hook point available to both clients and servers. It is - illegal for an interceptor to block/delay this operation */ + /* This is a special hook point available to both clients and servers when + TryCancel() is performed. It is illegal for an interceptor to block/delay + this operation. ALL interceptors see this hook point irrespective of + whether the RPC was hijacked or not. */ PRE_SEND_CANCEL, NUM_INTERCEPTION_HOOKS }; @@ -71,7 +73,7 @@ class InterceptorBatchMethods { // Calling this will signal that the interceptor is done intercepting the // current batch of the RPC. // Proceed is a no-op if the batch contains PRE_SEND_CANCEL. Simply returning - // from the Intercept method does the job of continuing the RPC. + // from the Intercept method does the job of continuing the RPC in this case. virtual void Proceed() = 0; // Calling this indicates that the interceptor has hijacked the RPC (only // valid if the batch contains send_initial_metadata on the client side) -- cgit v1.2.3 From 382fe4b4753e3cda6439b0a8febcd94807cd68c7 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 18:08:43 -0700 Subject: Add override back to GetInterceptedChannel --- include/grpcpp/impl/codegen/interceptor_common.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index f520e7905a..957d4f5ee7 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -145,7 +145,7 @@ class InterceptorBatchMethodsImpl recv_trailing_metadata_ = map; } - std::unique_ptr GetInterceptedChannel() { + std::unique_ptr GetInterceptedChannel() override { auto* info = call_->client_rpc_info(); if (info == nullptr) { return std::unique_ptr(nullptr); @@ -444,7 +444,7 @@ class CancelInterceptorBatchMethods return nullptr; } - std::unique_ptr GetInterceptedChannel() { + std::unique_ptr GetInterceptedChannel() override { GPR_CODEGEN_ASSERT(false && "It is illegal to call GetInterceptedChannel on a " "method which has a Cancel notification"); -- cgit v1.2.3 From ded9434e4c23e6be69feeda9ca05b31bceb5e5f9 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 1 Nov 2018 18:36:40 -0700 Subject: Add array header for complaining compiler --- include/grpcpp/impl/codegen/interceptor_common.h | 1 + 1 file changed, 1 insertion(+) diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 957d4f5ee7..d0aa23cb0a 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -19,6 +19,7 @@ #ifndef GRPCPP_IMPL_CODEGEN_INTERCEPTOR_COMMON_H #define GRPCPP_IMPL_CODEGEN_INTERCEPTOR_COMMON_H +#include #include #include -- cgit v1.2.3