From fc539eb19334bdfde8bc798d94ccb56b686e5193 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Wed, 21 Jun 2017 16:17:35 -0700 Subject: [PATCH 1/3] Re-enable public constructor for ClientAsyncResponseReader to avoid busting client that bypassed code generator. This code is deprecated-on-arrival as it is a performance pessimization. This code path should not be used. --- .../grpc++/impl/codegen/async_unary_call.h | 90 ++++++++++++++----- include/grpc++/impl/codegen/call.h | 20 +++++ 2 files changed, 89 insertions(+), 21 deletions(-) diff --git a/include/grpc++/impl/codegen/async_unary_call.h b/include/grpc++/impl/codegen/async_unary_call.h index 4aa4e25a9e9..de0ab2372a6 100644 --- a/include/grpc++/impl/codegen/async_unary_call.h +++ b/include/grpc++/impl/codegen/async_unary_call.h @@ -87,6 +87,28 @@ class ClientAsyncResponseReader final ClientAsyncResponseReader(call, context, request); } + /// TODO(vjpai): Delete the below constructor + /// PLEASE DO NOT USE THIS CONSTRUCTOR IN NEW CODE + /// This code is only present as a short-term workaround + /// for users that bypassed the code-generator and directly + /// created this struct rather than properly using a stub. + /// This code will not remain a valid public constructor for long. + template + ClientAsyncResponseReader(ChannelInterface* channel, CompletionQueue* cq, + const RpcMethod& method, ClientContext* context, + const W& request) + : context_(context), + call_(channel->CreateCall(method, context, cq)), + collection_(std::make_shared()) { + collection_->init_buf_.SetCollection(collection_); + collection_->init_buf_.SendInitialMetadata( + context->send_initial_metadata_, context->initial_metadata_flags()); + // TODO(ctiller): don't assert + GPR_CODEGEN_ASSERT(collection_->init_buf_.SendMessage(request).ok()); + collection_->init_buf_.ClientSendClose(); + call_.PerformOps(&collection_->init_buf_); + } + // always allocated against a call arena, no memory free required static void operator delete(void* ptr, std::size_t size) { assert(size == sizeof(ClientAsyncResponseReader)); @@ -101,9 +123,18 @@ class ClientAsyncResponseReader final void ReadInitialMetadata(void* tag) { GPR_CODEGEN_ASSERT(!context_->initial_metadata_received_); - meta_buf_.set_output_tag(tag); - meta_buf_.RecvInitialMetadata(context_); - call_.PerformOps(&meta_buf_); + Ops& o = ops; + + // TODO(vjpai): Remove the collection_ specialization as soon + // as the public constructor is deleted + if (collection_) { + o = *collection_; + collection_->meta_buf_.SetCollection(collection_); + } + + o.meta_buf_.set_output_tag(tag); + o.meta_buf_.RecvInitialMetadata(context_); + call_.PerformOps(&o.meta_buf_); } /// See \a ClientAysncResponseReaderInterface::Finish for semantics. @@ -112,14 +143,23 @@ class ClientAsyncResponseReader final /// - the \a ClientContext associated with this call is updated with /// possible initial and trailing metadata sent from the server. void Finish(R* msg, Status* status, void* tag) { - finish_buf_.set_output_tag(tag); + Ops& o = ops; + + // TODO(vjpai): Remove the collection_ specialization as soon + // as the public constructor is deleted + if (collection_) { + o = *collection_; + collection_->finish_buf_.SetCollection(collection_); + } + + o.finish_buf_.set_output_tag(tag); if (!context_->initial_metadata_received_) { - finish_buf_.RecvInitialMetadata(context_); + o.finish_buf_.RecvInitialMetadata(context_); } - finish_buf_.RecvMessage(msg); - finish_buf_.AllowNoMessage(); - finish_buf_.ClientRecvStatus(context_, status); - call_.PerformOps(&finish_buf_); + o.finish_buf_.RecvMessage(msg); + o.finish_buf_.AllowNoMessage(); + o.finish_buf_.ClientRecvStatus(context_, status); + call_.PerformOps(&o.finish_buf_); } private: @@ -129,25 +169,33 @@ class ClientAsyncResponseReader final template ClientAsyncResponseReader(Call call, ClientContext* context, const W& request) : context_(context), call_(call) { - init_buf_.SendInitialMetadata(context->send_initial_metadata_, - context->initial_metadata_flags()); + ops.init_buf_.SendInitialMetadata(context->send_initial_metadata_, + context->initial_metadata_flags()); // TODO(ctiller): don't assert - GPR_CODEGEN_ASSERT(init_buf_.SendMessage(request).ok()); - init_buf_.ClientSendClose(); - call_.PerformOps(&init_buf_); + GPR_CODEGEN_ASSERT(ops.init_buf_.SendMessage(request).ok()); + ops.init_buf_.ClientSendClose(); + call_.PerformOps(&ops.init_buf_); } // disable operator new static void* operator new(std::size_t size); static void* operator new(std::size_t size, void* p) { return p; } - SneakyCallOpSet - init_buf_; - CallOpSet meta_buf_; - CallOpSet, - CallOpClientRecvStatus> - finish_buf_; + // TODO(vjpai): Remove the reference to CallOpSetCollectionInterface + // as soon as the related workaround (public constructor) is deleted + struct Ops : public CallOpSetCollectionInterface { + SneakyCallOpSet + init_buf_; + CallOpSet meta_buf_; + CallOpSet, + CallOpClientRecvStatus> + finish_buf_; + } ops; + + // TODO(vjpai): Remove the collection_ as soon as the related workaround + // (public constructor) is deleted + std::shared_ptr collection_; }; /// Async server-side API for handling unary calls, where the single diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index 2ded2d97762..dba53bc4273 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -544,6 +544,11 @@ class CallOpClientRecvStatus { grpc_slice error_message_; }; +/// TODO(vjpai): Remove the existence of CallOpSetCollectionInterface +/// and references to it. This code is deprecated-on-arrival and is +/// only added for users that bypassed the code-generator. +class CallOpSetCollectionInterface {}; + /// An abstract collection of call ops, used to generate the /// grpc_call_op structure to pass down to the lower layers, /// and as it is-a CompletionQueueTag, also massages the final @@ -554,6 +559,18 @@ class CallOpSetInterface : public CompletionQueueTag { /// Fills in grpc_op, starting from ops[*nops] and moving /// upwards. virtual void FillOps(grpc_call* call, grpc_op* ops, size_t* nops) = 0; + + /// TODO(vjpai): Remove the SetCollection method and comment. This is only + /// a short-term workaround for users that bypassed the code generator + /// Mark this as belonging to a collection if needed + void SetCollection(std::shared_ptr collection) { + collection_ = collection; + } + + protected: + /// TODO(vjpai): Remove the collection_ field once the idea of bypassing the + /// code generator is forbidden. This is already deprecated + std::shared_ptr collection_; }; /// Primary implementaiton of CallOpSetInterface. @@ -594,6 +611,9 @@ class CallOpSet : public CallOpSetInterface, this->Op6::FinishOp(status); *tag = return_tag_; g_core_codegen_interface->grpc_call_unref(call_); + // TODO(vjpai): Remove the reference to collection_ once the idea of + // bypassing the code generator is forbidden. It is already deprecated + collection_.reset(); return true; } From d35730d18564a00a4b9acccaf8387ccb4bb565b8 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Thu, 22 Jun 2017 12:06:44 -0700 Subject: [PATCH 2/3] Fix use of terminal underscores in field names. --- .../grpc++/impl/codegen/async_unary_call.h | 52 +++++++++---------- 1 file changed, 26 insertions(+), 26 deletions(-) diff --git a/include/grpc++/impl/codegen/async_unary_call.h b/include/grpc++/impl/codegen/async_unary_call.h index de0ab2372a6..a5a698c6401 100644 --- a/include/grpc++/impl/codegen/async_unary_call.h +++ b/include/grpc++/impl/codegen/async_unary_call.h @@ -100,13 +100,13 @@ class ClientAsyncResponseReader final : context_(context), call_(channel->CreateCall(method, context, cq)), collection_(std::make_shared()) { - collection_->init_buf_.SetCollection(collection_); - collection_->init_buf_.SendInitialMetadata( + collection_->init_buf.SetCollection(collection_); + collection_->init_buf.SendInitialMetadata( context->send_initial_metadata_, context->initial_metadata_flags()); // TODO(ctiller): don't assert - GPR_CODEGEN_ASSERT(collection_->init_buf_.SendMessage(request).ok()); - collection_->init_buf_.ClientSendClose(); - call_.PerformOps(&collection_->init_buf_); + GPR_CODEGEN_ASSERT(collection_->init_buf.SendMessage(request).ok()); + collection_->init_buf.ClientSendClose(); + call_.PerformOps(&collection_->init_buf); } // always allocated against a call arena, no memory free required @@ -123,18 +123,18 @@ class ClientAsyncResponseReader final void ReadInitialMetadata(void* tag) { GPR_CODEGEN_ASSERT(!context_->initial_metadata_received_); - Ops& o = ops; + Ops& o = ops_; // TODO(vjpai): Remove the collection_ specialization as soon // as the public constructor is deleted if (collection_) { o = *collection_; - collection_->meta_buf_.SetCollection(collection_); + collection_->meta_buf.SetCollection(collection_); } - o.meta_buf_.set_output_tag(tag); - o.meta_buf_.RecvInitialMetadata(context_); - call_.PerformOps(&o.meta_buf_); + o.meta_buf.set_output_tag(tag); + o.meta_buf.RecvInitialMetadata(context_); + call_.PerformOps(&o.meta_buf); } /// See \a ClientAysncResponseReaderInterface::Finish for semantics. @@ -143,23 +143,23 @@ class ClientAsyncResponseReader final /// - the \a ClientContext associated with this call is updated with /// possible initial and trailing metadata sent from the server. void Finish(R* msg, Status* status, void* tag) { - Ops& o = ops; + Ops& o = ops_; // TODO(vjpai): Remove the collection_ specialization as soon // as the public constructor is deleted if (collection_) { o = *collection_; - collection_->finish_buf_.SetCollection(collection_); + collection_->finish_buf.SetCollection(collection_); } - o.finish_buf_.set_output_tag(tag); + o.finish_buf.set_output_tag(tag); if (!context_->initial_metadata_received_) { - o.finish_buf_.RecvInitialMetadata(context_); + o.finish_buf.RecvInitialMetadata(context_); } - o.finish_buf_.RecvMessage(msg); - o.finish_buf_.AllowNoMessage(); - o.finish_buf_.ClientRecvStatus(context_, status); - call_.PerformOps(&o.finish_buf_); + o.finish_buf.RecvMessage(msg); + o.finish_buf.AllowNoMessage(); + o.finish_buf.ClientRecvStatus(context_, status); + call_.PerformOps(&o.finish_buf); } private: @@ -169,12 +169,12 @@ class ClientAsyncResponseReader final template ClientAsyncResponseReader(Call call, ClientContext* context, const W& request) : context_(context), call_(call) { - ops.init_buf_.SendInitialMetadata(context->send_initial_metadata_, + ops_.init_buf.SendInitialMetadata(context->send_initial_metadata_, context->initial_metadata_flags()); // TODO(ctiller): don't assert - GPR_CODEGEN_ASSERT(ops.init_buf_.SendMessage(request).ok()); - ops.init_buf_.ClientSendClose(); - call_.PerformOps(&ops.init_buf_); + GPR_CODEGEN_ASSERT(ops_.init_buf.SendMessage(request).ok()); + ops_.init_buf.ClientSendClose(); + call_.PerformOps(&ops_.init_buf); } // disable operator new @@ -186,12 +186,12 @@ class ClientAsyncResponseReader final struct Ops : public CallOpSetCollectionInterface { SneakyCallOpSet - init_buf_; - CallOpSet meta_buf_; + init_buf; + CallOpSet meta_buf; CallOpSet, CallOpClientRecvStatus> - finish_buf_; - } ops; + finish_buf; + } ops_; // TODO(vjpai): Remove the collection_ as soon as the related workaround // (public constructor) is deleted From 10c040d455289b274fe2c5de00a9ffeb79798681 Mon Sep 17 00:00:00 2001 From: Vijay Pai Date: Thu, 22 Jun 2017 13:41:41 -0700 Subject: [PATCH 3/3] Move collection reset before unref (since unref could destroy obj) --- include/grpc++/impl/codegen/call.h | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/include/grpc++/impl/codegen/call.h b/include/grpc++/impl/codegen/call.h index dba53bc4273..342ea462033 100644 --- a/include/grpc++/impl/codegen/call.h +++ b/include/grpc++/impl/codegen/call.h @@ -610,10 +610,12 @@ class CallOpSet : public CallOpSetInterface, this->Op5::FinishOp(status); this->Op6::FinishOp(status); *tag = return_tag_; - g_core_codegen_interface->grpc_call_unref(call_); + // TODO(vjpai): Remove the reference to collection_ once the idea of // bypassing the code generator is forbidden. It is already deprecated collection_.reset(); + + g_core_codegen_interface->grpc_call_unref(call_); return true; }