From 4224384d398ee3ddd01c0b95f93f33bdd75e8fb2 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 3 Jan 2019 15:47:00 -0800 Subject: [PATCH 1/5] Modifying semantics for GetSendMessage and GetSerializedSendMessage. Also adding ModifySendMessage --- include/grpcpp/impl/codegen/call_op_set.h | 31 +++++--- include/grpcpp/impl/codegen/interceptor.h | 2 + .../grpcpp/impl/codegen/interceptor_common.h | 33 +++++++-- .../client_interceptors_end2end_test.cc | 8 +-- .../server_interceptors_end2end_test.cc | 71 +++++++++++++++---- 5 files changed, 115 insertions(+), 30 deletions(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 310bea93ca7..b9cf0b1db04 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -317,7 +317,10 @@ class CallOpSendMessage { protected: void AddOp(grpc_op* ops, size_t* nops) { - if (!send_buf_.Valid() || hijacked_) return; + if ((msg_ == nullptr && !send_buf_.Valid()) || hijacked_) return; + if (msg_ != nullptr) { + GPR_CODEGEN_ASSERT(serializer_(msg_).ok()); + } grpc_op* op = &ops[(*nops)++]; op->op = GRPC_OP_SEND_MESSAGE; op->flags = write_options_.flags(); @@ -330,17 +333,17 @@ class CallOpSendMessage { void SetInterceptionHookPoint( InterceptorBatchMethodsImpl* interceptor_methods) { - if (!send_buf_.Valid()) return; + if (msg_ == nullptr && !send_buf_.Valid()) return; interceptor_methods->AddInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE); - interceptor_methods->SetSendMessage(&send_buf_, msg_); + interceptor_methods->SetSendMessage(&send_buf_, &msg_, serializer_); } void SetFinishInterceptionHookPoint( InterceptorBatchMethodsImpl* interceptor_methods) { // The contents of the SendMessage value that was previously set // has had its references stolen by core's operations - interceptor_methods->SetSendMessage(nullptr, nullptr); + interceptor_methods->SetSendMessage(nullptr, nullptr, nullptr); } void SetHijackingState(InterceptorBatchMethodsImpl* interceptor_methods) { @@ -352,6 +355,7 @@ class CallOpSendMessage { bool hijacked_ = false; ByteBuffer send_buf_; WriteOptions write_options_; + std::function serializer_; }; template @@ -362,12 +366,21 @@ Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { // The void in the template parameter below should not be needed // (since it should be implicit) but is needed due to an observed // difference in behavior between clang and gcc for certain internal users - Status result = SerializationTraits::Serialize( - message, send_buf_.bbuf_ptr(), &own_buf); - if (!own_buf) { - send_buf_.Duplicate(); + serializer_ = [this](const void* message) { + bool own_buf; + send_buf_.Clear(); + Status result = SerializationTraits::Serialize( + *static_cast(message), send_buf_.bbuf_ptr(), &own_buf); + if (!own_buf) { + send_buf_.Duplicate(); + } + return result; + }; + // Serialize immediately only if we do not have access to the message pointer + if (msg_ == nullptr) { + return serializer_(&message); } - return result; + return Status::OK; } template diff --git a/include/grpcpp/impl/codegen/interceptor.h b/include/grpcpp/impl/codegen/interceptor.h index 5a9a3a44e6e..edf3ab49f16 100644 --- a/include/grpcpp/impl/codegen/interceptor.h +++ b/include/grpcpp/impl/codegen/interceptor.h @@ -117,6 +117,8 @@ class InterceptorBatchMethods { /// only supported for sync and callback APIs at the present moment. virtual const void* GetSendMessage() = 0; + virtual void ModifySendMessage(const void* message) = 0; + /// Returns a modifiable multimap of the initial metadata to be sent. Valid /// for PRE_SEND_INITIAL_METADATA interceptions. A value of nullptr indicates /// that this field is not valid. diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 4b7eaefee1b..6fa6210dc36 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -79,9 +79,24 @@ class InterceptorBatchMethodsImpl hooks_[static_cast(type)] = true; } - ByteBuffer* GetSerializedSendMessage() override { return send_message_; } + ByteBuffer* GetSerializedSendMessage() override { + GPR_CODEGEN_ASSERT(orig_send_message_ != nullptr); + if (*orig_send_message_ != nullptr) { + GPR_CODEGEN_ASSERT(serializer_(*orig_send_message_).ok()); + *orig_send_message_ = nullptr; + } + return send_message_; + } + + const void* GetSendMessage() override { + GPR_CODEGEN_ASSERT(orig_send_message_ != nullptr); + return *orig_send_message_; + } - const void* GetSendMessage() override { return orig_send_message_; } + void ModifySendMessage(const void* message) override { + GPR_CODEGEN_ASSERT(orig_send_message_ != nullptr); + *orig_send_message_ = message; + } std::multimap* GetSendInitialMetadata() override { return send_initial_metadata_; @@ -117,9 +132,11 @@ class InterceptorBatchMethodsImpl return recv_trailing_metadata_->map(); } - void SetSendMessage(ByteBuffer* buf, const void* msg) { + void SetSendMessage(ByteBuffer* buf, const void** msg, + std::function serializer) { send_message_ = buf; orig_send_message_ = msg; + serializer_ = serializer; } void SetSendInitialMetadata( @@ -339,7 +356,8 @@ class InterceptorBatchMethodsImpl std::function callback_; ByteBuffer* send_message_ = nullptr; - const void* orig_send_message_ = nullptr; + const void** orig_send_message_ = nullptr; + std::function serializer_; std::multimap* send_initial_metadata_; @@ -400,6 +418,13 @@ class CancelInterceptorBatchMethods return nullptr; } + void ModifySendMessage(const void* message) override { + GPR_CODEGEN_ASSERT( + false && + "It is illegal to call ModifySendMessage on a method which " + "has a Cancel notification"); + } + std::multimap* GetSendInitialMetadata() override { GPR_CODEGEN_ASSERT(false && "It is illegal to call GetSendInitialMetadata on a " diff --git a/test/cpp/end2end/client_interceptors_end2end_test.cc b/test/cpp/end2end/client_interceptors_end2end_test.cc index 3db709e956e..ea72c1eda88 100644 --- a/test/cpp/end2end/client_interceptors_end2end_test.cc +++ b/test/cpp/end2end/client_interceptors_end2end_test.cc @@ -287,16 +287,16 @@ class LoggingInterceptor : public experimental::Interceptor { if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)) { EchoRequest req; + EXPECT_EQ(static_cast(methods->GetSendMessage()) + ->message() + .find("Hello"), + 0u); auto* buffer = methods->GetSerializedSendMessage(); auto copied_buffer = *buffer; EXPECT_TRUE( SerializationTraits::Deserialize(&copied_buffer, &req) .ok()); EXPECT_TRUE(req.message().find("Hello") == 0u); - EXPECT_EQ(static_cast(methods->GetSendMessage()) - ->message() - .find("Hello"), - 0u); } if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_CLOSE)) { diff --git a/test/cpp/end2end/server_interceptors_end2end_test.cc b/test/cpp/end2end/server_interceptors_end2end_test.cc index 09e855b0d01..82f142ba913 100644 --- a/test/cpp/end2end/server_interceptors_end2end_test.cc +++ b/test/cpp/end2end/server_interceptors_end2end_test.cc @@ -142,29 +142,68 @@ class LoggingInterceptorFactory } }; -// Test if GetSendMessage works as expected -class GetSendMessageTester : public experimental::Interceptor { +// Test if SendMessage function family works as expected for sync/callback apis +class SyncSendMessageTester : public experimental::Interceptor { public: - GetSendMessageTester(experimental::ServerRpcInfo* info) {} + SyncSendMessageTester(experimental::ServerRpcInfo* info) {} void Intercept(experimental::InterceptorBatchMethods* methods) override { if (methods->QueryInterceptionHookPoint( experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)) { - EXPECT_EQ(static_cast(methods->GetSendMessage()) - ->message() - .find("Hello"), - 0u); + string old_msg = + static_cast(methods->GetSendMessage())->message(); + EXPECT_EQ(old_msg.find("Hello"), 0u); + new_msg_.set_message("World" + old_msg); + methods->ModifySendMessage(&new_msg_); } methods->Proceed(); } + + private: + EchoRequest new_msg_; }; -class GetSendMessageTesterFactory +class SyncSendMessageTesterFactory : public experimental::ServerInterceptorFactoryInterface { public: virtual experimental::Interceptor* CreateServerInterceptor( experimental::ServerRpcInfo* info) override { - return new GetSendMessageTester(info); + return new SyncSendMessageTester(info); + } +}; + +// Test if SendMessage function family works as expected for sync/callback apis +class SyncSendMessageVerifier : public experimental::Interceptor { + public: + SyncSendMessageVerifier(experimental::ServerRpcInfo* info) {} + + void Intercept(experimental::InterceptorBatchMethods* methods) override { + if (methods->QueryInterceptionHookPoint( + experimental::InterceptionHookPoints::PRE_SEND_MESSAGE)) { + // Make sure that the changes made in SyncSendMessageTester persisted + string old_msg = + static_cast(methods->GetSendMessage())->message(); + EXPECT_EQ(old_msg.find("World"), 0u); + + // Remove the "World" part of the string that we added earlier + new_msg_.set_message(old_msg.erase(0, 5)); + methods->ModifySendMessage(&new_msg_); + + // LoggingInterceptor verifies that changes got reverted + } + methods->Proceed(); + } + + private: + EchoRequest new_msg_; +}; + +class SyncSendMessageVerifierFactory + : public experimental::ServerInterceptorFactoryInterface { + public: + virtual experimental::Interceptor* CreateServerInterceptor( + experimental::ServerRpcInfo* info) override { + return new SyncSendMessageVerifier(info); } }; @@ -201,10 +240,13 @@ class ServerInterceptorsEnd2endSyncUnaryTest : public ::testing::Test { creators; creators.push_back( std::unique_ptr( - new LoggingInterceptorFactory())); + new SyncSendMessageTesterFactory())); creators.push_back( std::unique_ptr( - new GetSendMessageTesterFactory())); + new SyncSendMessageVerifierFactory())); + creators.push_back( + std::unique_ptr( + new LoggingInterceptorFactory())); // Add 20 dummy interceptor factories and null interceptor factories for (auto i = 0; i < 20; i++) { creators.push_back(std::unique_ptr( @@ -244,10 +286,13 @@ class ServerInterceptorsEnd2endSyncStreamingTest : public ::testing::Test { creators; creators.push_back( std::unique_ptr( - new LoggingInterceptorFactory())); + new SyncSendMessageTesterFactory())); creators.push_back( std::unique_ptr( - new GetSendMessageTesterFactory())); + new SyncSendMessageVerifierFactory())); + creators.push_back( + std::unique_ptr( + new LoggingInterceptorFactory())); for (auto i = 0; i < 20; i++) { creators.push_back(std::unique_ptr( new DummyInterceptorFactory())); From df49204b975b898392655a4af5a0597d7b2ec850 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 3 Jan 2019 18:10:21 -0800 Subject: [PATCH 2/5] Remove unused variable --- include/grpcpp/impl/codegen/call_op_set.h | 1 - 1 file changed, 1 deletion(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index b9cf0b1db04..88c744a3fcf 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -361,7 +361,6 @@ class CallOpSendMessage { template Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { write_options_ = options; - bool own_buf; // TODO(vjpai): Remove the void below when possible // The void in the template parameter below should not be needed // (since it should be implicit) but is needed due to an observed From 2b4781ca526b0823fbea263a5c7b07fdf8abe41d Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 3 Jan 2019 18:32:10 -0800 Subject: [PATCH 3/5] Use Status() instead of Status::OK to avoid issues with codegen_test_minimal --- include/grpcpp/impl/codegen/call_op_set.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 88c744a3fcf..e8a4d389f17 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -379,7 +379,7 @@ Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { if (msg_ == nullptr) { return serializer_(&message); } - return Status::OK; + return Status(); } template From 7d1491d64c9b6279233c780d290c514a7040c1c7 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 7 Jan 2019 09:23:35 -0800 Subject: [PATCH 4/5] Address reviewer comments --- include/grpcpp/impl/codegen/call_op_set.h | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index e8a4d389f17..ced0b2ff3e6 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -321,6 +321,7 @@ class CallOpSendMessage { if (msg_ != nullptr) { GPR_CODEGEN_ASSERT(serializer_(msg_).ok()); } + serializer_ = nullptr; grpc_op* op = &ops[(*nops)++]; op->op = GRPC_OP_SEND_MESSAGE; op->flags = write_options_.flags(); @@ -361,13 +362,13 @@ class CallOpSendMessage { template Status CallOpSendMessage::SendMessage(const M& message, WriteOptions options) { write_options_ = options; - // TODO(vjpai): Remove the void below when possible - // The void in the template parameter below should not be needed - // (since it should be implicit) but is needed due to an observed - // difference in behavior between clang and gcc for certain internal users serializer_ = [this](const void* message) { bool own_buf; send_buf_.Clear(); + // TODO(vjpai): Remove the void below when possible + // The void in the template parameter below should not be needed + // (since it should be implicit) but is needed due to an observed + // difference in behavior between clang and gcc for certain internal users Status result = SerializationTraits::Serialize( *static_cast(message), send_buf_.bbuf_ptr(), &own_buf); if (!own_buf) { From 34d77aae5ec26063e2eb5dc4b47ba4dce90c7136 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Mon, 7 Jan 2019 14:12:03 -0800 Subject: [PATCH 5/5] Always nullify serializer to free memory --- include/grpcpp/impl/codegen/call_op_set.h | 2 +- include/grpcpp/impl/codegen/interceptor_common.h | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/include/grpcpp/impl/codegen/call_op_set.h b/include/grpcpp/impl/codegen/call_op_set.h index 2c34082ebb1..880c62344b8 100644 --- a/include/grpcpp/impl/codegen/call_op_set.h +++ b/include/grpcpp/impl/codegen/call_op_set.h @@ -324,8 +324,8 @@ class CallOpSendMessage { } if (msg_ != nullptr) { GPR_CODEGEN_ASSERT(serializer_(msg_).ok()); - serializer_ = nullptr; } + serializer_ = nullptr; grpc_op* op = &ops[(*nops)++]; op->op = GRPC_OP_SEND_MESSAGE; op->flags = write_options_.flags(); diff --git a/include/grpcpp/impl/codegen/interceptor_common.h b/include/grpcpp/impl/codegen/interceptor_common.h index 33e46389b3d..09721343ffa 100644 --- a/include/grpcpp/impl/codegen/interceptor_common.h +++ b/include/grpcpp/impl/codegen/interceptor_common.h @@ -98,9 +98,7 @@ class InterceptorBatchMethodsImpl *orig_send_message_ = message; } - bool GetSendMessageStatus() override { - return !*fail_send_message_; - } + bool GetSendMessageStatus() override { return !*fail_send_message_; } std::multimap* GetSendInitialMetadata() override { return send_initial_metadata_;