From cc21d32c7797348c6be815357f8f97ac306fdd19 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Fri, 26 Oct 2018 00:13:55 -0700 Subject: [PATCH] Some cleanup --- include/grpcpp/impl/codegen/call.h | 23 ++++++++----------- include/grpcpp/impl/codegen/call_wrapper.h | 2 +- .../grpcpp/impl/codegen/intercepted_channel.h | 4 ++++ include/grpcpp/impl/codegen/interceptor.h | 4 ---- src/cpp/server/server_cc.cc | 1 - .../client_interceptors_end2end_test.cc | 1 - .../server_interceptors_end2end_test.cc | 1 - 7 files changed, 14 insertions(+), 22 deletions(-) diff --git a/include/grpcpp/impl/codegen/call.h b/include/grpcpp/impl/codegen/call.h index 0a6dad9cd8f..ef6a5178027 100644 --- a/include/grpcpp/impl/codegen/call.h +++ b/include/grpcpp/impl/codegen/call.h @@ -207,6 +207,7 @@ class WriteOptions { namespace internal { +/// Internal methods for setting the state class InternalInterceptorBatchMethods : public experimental::InterceptorBatchMethods { public: @@ -247,7 +248,6 @@ class CallNoOp { void FinishOp(bool* status) {} void SetInterceptionHookPoint( InternalInterceptorBatchMethods* interceptor_methods) {} - void SetFinishInterceptionHookPoint( InternalInterceptorBatchMethods* interceptor_methods) {} void SetHijackingState(InternalInterceptorBatchMethods* interceptor_methods) { @@ -846,6 +846,8 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { // Only the client can hijack when sending down initial metadata GPR_CODEGEN_ASSERT(!reverse_ && ops_ != nullptr && call_->client_rpc_info() != nullptr); + // It is illegal to call Hijack twice + GPR_CODEGEN_ASSERT(!ran_hijacking_interceptor_); auto* rpc_info = call_->client_rpc_info(); rpc_info->hijacked_ = true; rpc_info->hijacked_interceptor_ = curr_iteration_; @@ -960,6 +962,8 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { // This needs to be set before interceptors are run void SetCall(Call* call) { call_ = call; } + // This needs to be set before interceptors are run using RunInterceptors(). + // Alternatively, RunInterceptors(std::function f) can be used. void SetCallOpSetInterface(CallOpSetInterface* ops) { ops_ = ops; } // Returns true if no interceptors are run. This should be used only by @@ -969,6 +973,7 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { // ContinueFinalizeOpsAfterInterception will be called. Note that neither of // them is invoked if there were no interceptors registered. bool RunInterceptors() { + GPR_CODEGEN_ASSERT(ops_); auto* client_rpc_info = call_->client_rpc_info(); if (client_rpc_info != nullptr) { if (client_rpc_info->interceptors_.size() == 0) { @@ -990,9 +995,10 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { // Returns true if no interceptors are run. Returns false otherwise if there // are interceptors registered. After the interceptors are done running \a f - // will - // be invoked. This is to be used only by BaseAsyncRequest and SyncRequest. + // will be invoked. This is to be used only by BaseAsyncRequest and + // SyncRequest. bool RunInterceptors(std::function f) { + // This is used only by the server for initial call request GPR_CODEGEN_ASSERT(reverse_ == true); GPR_CODEGEN_ASSERT(call_->client_rpc_info() == nullptr); auto* server_rpc_info = call_->server_rpc_info(); @@ -1013,8 +1019,6 @@ class InterceptorBatchMethodsImpl : public InternalInterceptorBatchMethods { } else { if (rpc_info->hijacked_) { curr_iteration_ = rpc_info->hijacked_interceptor_; - // gpr_log(GPR_ERROR, "running from the hijacked %d", - // rpc_info->hijacked_interceptor_); } else { curr_iteration_ = rpc_info->interceptors_.size() - 1; } @@ -1173,14 +1177,12 @@ class CallOpSet : public CallOpSetInterface, } void FillOps(Call* call) override { - // gpr_log(GPR_ERROR, "filling ops %p", this); done_intercepting_ = false; g_core_codegen_interface->grpc_call_ref(call->call()); call_ = *call; // It's fine to create a copy of call since it's just pointers if (RunInterceptors()) { - // gpr_log(GPR_ERROR, "no interceptors on send path"); ContinueFillOpsAfterInterception(); } else { // After the interceptors are run, ContinueFillOpsAfterInterception will @@ -1193,7 +1195,6 @@ class CallOpSet : public CallOpSetInterface, // We have already finished intercepting and filling in the results. This // round trip from the core needed to be made because interceptors were // run - // gpr_log(GPR_ERROR, "done intercepting"); *tag = return_tag_; *status = saved_status_; g_core_codegen_interface->grpc_call_unref(call_.call()); @@ -1207,14 +1208,11 @@ class CallOpSet : public CallOpSetInterface, this->Op5::FinishOp(status); this->Op6::FinishOp(status); saved_status_ = *status; - // gpr_log(GPR_ERROR, "done finish ops"); if (RunInterceptorsPostRecv()) { *tag = return_tag_; g_core_codegen_interface->grpc_call_unref(call_.call()); - // gpr_log(GPR_ERROR, "no interceptors"); return true; } - // gpr_log(GPR_ERROR, "running interceptors"); // Interceptors are going to be run, so we can't return the tag just yet. // After the interceptors are run, ContinueFinalizeResultAfterInterception return false; @@ -1252,8 +1250,6 @@ class CallOpSet : public CallOpSetInterface, this->Op4::AddOp(ops, &nops); this->Op5::AddOp(ops, &nops); this->Op6::AddOp(ops, &nops); - // gpr_log(GPR_ERROR, "going to start call batch %p with %lu ops", this, - // nops); GPR_CODEGEN_ASSERT(GRPC_CALL_OK == g_core_codegen_interface->grpc_call_start_batch( call_.call(), ops, nops, cq_tag(), nullptr)); @@ -1263,7 +1259,6 @@ class CallOpSet : public CallOpSetInterface, // path void ContinueFinalizeResultAfterInterception() override { done_intercepting_ = true; - // gpr_log(GPR_ERROR, "going to start call batch %p for dummy tag", this); GPR_CODEGEN_ASSERT(GRPC_CALL_OK == g_core_codegen_interface->grpc_call_start_batch( call_.call(), nullptr, 0, cq_tag(), nullptr)); diff --git a/include/grpcpp/impl/codegen/call_wrapper.h b/include/grpcpp/impl/codegen/call_wrapper.h index 675e36feb96..06ab1fbc1ca 100644 --- a/include/grpcpp/impl/codegen/call_wrapper.h +++ b/include/grpcpp/impl/codegen/call_wrapper.h @@ -88,4 +88,4 @@ class Call final { } // namespace internal } // namespace grpc -#endif // GRPCPP_IMPL_CODEGEN_CALL_WRAPPER_H \ No newline at end of file +#endif // GRPCPP_IMPL_CODEGEN_CALL_WRAPPER_H diff --git a/include/grpcpp/impl/codegen/intercepted_channel.h b/include/grpcpp/impl/codegen/intercepted_channel.h index 91d9cd84e3d..dd4b2d87124 100644 --- a/include/grpcpp/impl/codegen/intercepted_channel.h +++ b/include/grpcpp/impl/codegen/intercepted_channel.h @@ -27,6 +27,10 @@ namespace internal { class InterceptorBatchMethodsImpl; +/// An InterceptedChannel is available to client Interceptors. An +/// InterceptedChannel is unique to an interceptor, and when an RPC is started +/// on this channel, only those interceptors that come after this interceptor +/// see the RPC. class InterceptedChannel : public ChannelInterface { public: virtual ~InterceptedChannel() { channel_ = nullptr; } diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 2027fd69b19..cdd34b80d17 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -26,10 +26,6 @@ #include #include -// struct grpc_byte_buffer; -// struct grpc_status_code; -// struct grpc_metadata; - namespace grpc { class Status; diff --git a/src/cpp/server/server_cc.cc b/src/cpp/server/server_cc.cc index 6da4b32406c..59a531e2725 100644 --- a/src/cpp/server/server_cc.cc +++ b/src/cpp/server/server_cc.cc @@ -24,7 +24,6 @@ #include #include #include -#include #include #include #include diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index ff012f6f482..5720e87478f 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -306,7 +306,6 @@ class HijackingInterceptorMakesAnotherCall : public experimental::Interceptor { } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_RECV_INITIAL_METADATA)) { - gpr_log(GPR_ERROR, "hijacked"); auto* map = methods->GetRecvInitialMetadata(); // Got nothing better to do here at the moment EXPECT_EQ(map->size(), static_cast(0)); diff --git a/test/cpp/end2end/server_interceptors_end2end_test.cc b/test/cpp/end2end/server_interceptors_end2end_test.cc index 3c7e0df0426..44ba2a60093 100644 --- a/test/cpp/end2end/server_interceptors_end2end_test.cc +++ b/test/cpp/end2end/server_interceptors_end2end_test.cc @@ -92,7 +92,6 @@ class LoggingInterceptor : public experimental::Interceptor { LoggingInterceptor(experimental::ServerRpcInfo* info) { info_ = info; } virtual void Intercept(experimental::InterceptorBatchMethods* methods) { - // gpr_log(GPR_ERROR, "ran this"); if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_INITIAL_METADATA)) { auto* map = methods->GetSendInitialMetadata();