From aa1cd147291cb57665eccf2dc9baeaa39184f0ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 10:24:59 -0800 Subject: [PATCH 1/4] Clean up C++ filter API. --- src/core/lib/channel/channel_stack.h | 7 +++++ src/cpp/common/channel_filter.h | 40 +++++++++++--------------- test/cpp/common/channel_filter_test.cc | 17 ++++++++--- 3 files changed, 36 insertions(+), 28 deletions(-) diff --git a/src/core/lib/channel/channel_stack.h b/src/core/lib/channel/channel_stack.h index 5d064c5695b..d9d3a852335 100644 --- a/src/core/lib/channel/channel_stack.h +++ b/src/core/lib/channel/channel_stack.h @@ -34,6 +34,13 @@ #ifndef GRPC_CORE_LIB_CHANNEL_CHANNEL_STACK_H #define GRPC_CORE_LIB_CHANNEL_CHANNEL_STACK_H +////////////////////////////////////////////////////////////////////////////// +// IMPORTANT NOTE: +// +// When you update this API, please make the corresponding changes to +// the C++ API in src/cpp/common/channel_filter.{h,cc} +////////////////////////////////////////////////////////////////////////////// + /* A channel filter defines how operations on a channel are implemented. Channel filters are chained together to create full channels, and if those chains are linear, then channel stacks provide a mechanism to minimize diff --git a/src/cpp/common/channel_filter.h b/src/cpp/common/channel_filter.h index 107522ea04a..93efe0fc3bf 100644 --- a/src/cpp/common/channel_filter.h +++ b/src/cpp/common/channel_filter.h @@ -217,14 +217,13 @@ class TransportStreamOp { class ChannelData { public: virtual ~ChannelData() { - if (peer_) gpr_free((void *)peer_); } /// Initializes the call data. - virtual grpc_error *Init() { return GRPC_ERROR_NONE; } - - /// Caller does NOT take ownership of result. - const char *peer() const { return peer_; } + virtual grpc_error *Init(grpc_exec_ctx *exec_ctx, + grpc_channel_element_args *args) { + return GRPC_ERROR_NONE; + } // TODO(roth): Find a way to avoid passing elem into these methods. @@ -235,11 +234,7 @@ class ChannelData { const grpc_channel_info *channel_info); protected: - /// Takes ownership of \a peer. - ChannelData(const grpc_channel_args &args, const char *peer) : peer_(peer) {} - - private: - const char *peer_; + ChannelData() {} }; /// Represents call data. @@ -248,7 +243,10 @@ class CallData { virtual ~CallData() {} /// Initializes the call data. - virtual grpc_error *Init() { return GRPC_ERROR_NONE; } + virtual grpc_error *Init(grpc_exec_ctx *exec_ctx, ChannelData *channel_data, + grpc_channel_element_args *args) { + return GRPC_ERROR_NONE; + } // TODO(roth): Find a way to avoid passing elem into these methods. @@ -266,7 +264,7 @@ class CallData { virtual char *GetPeer(grpc_exec_ctx *exec_ctx, grpc_call_element *elem); protected: - explicit CallData(const ChannelData &) {} + CallData() {} }; namespace internal { @@ -282,14 +280,8 @@ class ChannelFilter final { static grpc_error *InitChannelElement(grpc_exec_ctx *exec_ctx, grpc_channel_element *elem, grpc_channel_element_args *args) { - const char *peer = - args->optional_transport - ? grpc_transport_get_peer(exec_ctx, args->optional_transport) - : nullptr; - // Construct the object in the already-allocated memory. - ChannelDataType *channel_data = - new (elem->channel_data) ChannelDataType(*args->channel_args, peer); - return channel_data->Init(); + ChannelDataType *channel_data = new (elem->channel_data) ChannelDataType(); + return channel_data->Init(exec_ctx, args); } static void DestroyChannelElement(grpc_exec_ctx *exec_ctx, @@ -317,11 +309,11 @@ class ChannelFilter final { static grpc_error *InitCallElement(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { - const ChannelDataType &channel_data = - *(ChannelDataType *)elem->channel_data; + ChannelDataType *channel_data = + (ChannelDataType *)elem->channel_data; // Construct the object in the already-allocated memory. - CallDataType *call_data = new (elem->call_data) CallDataType(channel_data); - return call_data->Init(); + CallDataType *call_data = new (elem->call_data) CallDataType(); + return call_data->Init(exec_ctx, channel_data, args); } static void DestroyCallElement(grpc_exec_ctx *exec_ctx, diff --git a/test/cpp/common/channel_filter_test.cc b/test/cpp/common/channel_filter_test.cc index 600a953d828..26d341c2b9f 100644 --- a/test/cpp/common/channel_filter_test.cc +++ b/test/cpp/common/channel_filter_test.cc @@ -41,14 +41,23 @@ namespace testing { class MyChannelData : public ChannelData { public: - MyChannelData(const grpc_channel_args& args, const char* peer) - : ChannelData(args, peer) {} + MyChannelData() {} + + grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) { + (void)args->channel_args; // Make sure field is available. + return GRPC_ERROR_NONE; + } }; class MyCallData : public CallData { public: - explicit MyCallData(const ChannelData& channel_data) - : CallData(channel_data) {} + MyCallData() {} + + grpc_error *Init(grpc_exec_ctx *exec_ctx, ChannelData *channel_data, + grpc_call_element_args *args) { + (void)args->path; // Make sure field is available. + return GRPC_ERROR_NONE; + } }; // This test ensures that when we make changes to the filter API in From 42663fb20ec0dc275dbc290c32969f48f5df6f7c Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 12 Dec 2016 10:51:01 -0800 Subject: [PATCH 2/4] Fix bug. --- src/cpp/common/channel_filter.h | 2 +- test/cpp/common/channel_filter_test.cc | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cpp/common/channel_filter.h b/src/cpp/common/channel_filter.h index 93efe0fc3bf..f4652cee771 100644 --- a/src/cpp/common/channel_filter.h +++ b/src/cpp/common/channel_filter.h @@ -244,7 +244,7 @@ class CallData { /// Initializes the call data. virtual grpc_error *Init(grpc_exec_ctx *exec_ctx, ChannelData *channel_data, - grpc_channel_element_args *args) { + grpc_call_element_args *args) { return GRPC_ERROR_NONE; } diff --git a/test/cpp/common/channel_filter_test.cc b/test/cpp/common/channel_filter_test.cc index 26d341c2b9f..0859cc024ba 100644 --- a/test/cpp/common/channel_filter_test.cc +++ b/test/cpp/common/channel_filter_test.cc @@ -43,7 +43,8 @@ class MyChannelData : public ChannelData { public: MyChannelData() {} - grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) { + grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) + override { (void)args->channel_args; // Make sure field is available. return GRPC_ERROR_NONE; } @@ -54,7 +55,7 @@ class MyCallData : public CallData { MyCallData() {} grpc_error *Init(grpc_exec_ctx *exec_ctx, ChannelData *channel_data, - grpc_call_element_args *args) { + grpc_call_element_args *args) override { (void)args->path; // Make sure field is available. return GRPC_ERROR_NONE; } From 473ff83facfadc823523d3f833924c661432a943 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Dec 2016 08:21:12 -0800 Subject: [PATCH 3/4] Fix filter_end2end_test. --- test/cpp/end2end/filter_end2end_test.cc | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/test/cpp/end2end/filter_end2end_test.cc b/test/cpp/end2end/filter_end2end_test.cc index ab6ed46de56..28bd6baab8b 100644 --- a/test/cpp/end2end/filter_end2end_test.cc +++ b/test/cpp/end2end/filter_end2end_test.cc @@ -114,20 +114,17 @@ int GetCallCounterValue() { class ChannelDataImpl : public ChannelData { public: - ChannelDataImpl(const grpc_channel_args& args, const char* peer) - : ChannelData(args, peer) { + grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) { IncrementConnectionCounter(); + return GRPC_ERROR_NONE; } }; class CallDataImpl : public CallData { public: - explicit CallDataImpl(const ChannelDataImpl& channel_data) - : CallData(channel_data) {} - void StartTransportStreamOp(grpc_exec_ctx* exec_ctx, grpc_call_element* elem, TransportStreamOp* op) override { - // Incrementing the counter could be done from the ctor, but we want + // Incrementing the counter could be done from Init(), but we want // to test that the individual methods are actually called correctly. if (op->recv_initial_metadata() != nullptr) IncrementCallCounter(); grpc_call_next_op(exec_ctx, elem, op->op()); From 13f35746426b9dcca3d751b3f7f298859ae18936 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 14 Dec 2016 12:39:03 -0800 Subject: [PATCH 4/4] clang-format --- src/cpp/common/channel_filter.h | 6 ++---- test/cpp/common/channel_filter_test.cc | 8 ++++---- test/cpp/end2end/filter_end2end_test.cc | 2 +- 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/src/cpp/common/channel_filter.h b/src/cpp/common/channel_filter.h index f4652cee771..c9f50df7321 100644 --- a/src/cpp/common/channel_filter.h +++ b/src/cpp/common/channel_filter.h @@ -216,8 +216,7 @@ class TransportStreamOp { /// Represents channel data. class ChannelData { public: - virtual ~ChannelData() { - } + virtual ~ChannelData() {} /// Initializes the call data. virtual grpc_error *Init(grpc_exec_ctx *exec_ctx, @@ -309,8 +308,7 @@ class ChannelFilter final { static grpc_error *InitCallElement(grpc_exec_ctx *exec_ctx, grpc_call_element *elem, grpc_call_element_args *args) { - ChannelDataType *channel_data = - (ChannelDataType *)elem->channel_data; + ChannelDataType *channel_data = (ChannelDataType *)elem->channel_data; // Construct the object in the already-allocated memory. CallDataType *call_data = new (elem->call_data) CallDataType(); return call_data->Init(exec_ctx, channel_data, args); diff --git a/test/cpp/common/channel_filter_test.cc b/test/cpp/common/channel_filter_test.cc index 0859cc024ba..32246a4b765 100644 --- a/test/cpp/common/channel_filter_test.cc +++ b/test/cpp/common/channel_filter_test.cc @@ -43,8 +43,8 @@ class MyChannelData : public ChannelData { public: MyChannelData() {} - grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) - override { + grpc_error* Init(grpc_exec_ctx* exec_ctx, + grpc_channel_element_args* args) override { (void)args->channel_args; // Make sure field is available. return GRPC_ERROR_NONE; } @@ -54,8 +54,8 @@ class MyCallData : public CallData { public: MyCallData() {} - grpc_error *Init(grpc_exec_ctx *exec_ctx, ChannelData *channel_data, - grpc_call_element_args *args) override { + grpc_error* Init(grpc_exec_ctx* exec_ctx, ChannelData* channel_data, + grpc_call_element_args* args) override { (void)args->path; // Make sure field is available. return GRPC_ERROR_NONE; } diff --git a/test/cpp/end2end/filter_end2end_test.cc b/test/cpp/end2end/filter_end2end_test.cc index 28bd6baab8b..bd384f68b40 100644 --- a/test/cpp/end2end/filter_end2end_test.cc +++ b/test/cpp/end2end/filter_end2end_test.cc @@ -114,7 +114,7 @@ int GetCallCounterValue() { class ChannelDataImpl : public ChannelData { public: - grpc_error *Init(grpc_exec_ctx *exec_ctx, grpc_channel_element_args *args) { + grpc_error* Init(grpc_exec_ctx* exec_ctx, grpc_channel_element_args* args) { IncrementConnectionCounter(); return GRPC_ERROR_NONE; }