From 5a5734ff0314b209e18eba5fda6ee92d6b650e56 Mon Sep 17 00:00:00 2001 From: Esun Kim Date: Wed, 6 Nov 2019 21:10:49 -0800 Subject: [PATCH] Use std::string for channelz --- .../client_channel/client_channel_channelz.cc | 14 +++---- .../client_channel/client_channel_channelz.h | 6 ++- .../transport/chttp2/server/chttp2_server.cc | 6 ++- .../chttp2/transport/chttp2_transport.cc | 7 ++-- src/core/lib/channel/channelz.cc | 41 +++++++++---------- src/core/lib/channel/channelz.h | 25 +++++------ src/core/lib/gprpp/string_view.h | 10 +++++ src/core/lib/surface/channel.cc | 6 +-- test/core/channel/channel_trace_test.cc | 20 ++++----- test/core/channel/channelz_registry_test.cc | 3 +- 10 files changed, 76 insertions(+), 62 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.cc b/src/core/ext/filters/client_channel/client_channel_channelz.cc index a47766031a3..bc9bbe59219 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.cc +++ b/src/core/ext/filters/client_channel/client_channel_channelz.cc @@ -30,11 +30,10 @@ namespace grpc_core { namespace channelz { -SubchannelNode::SubchannelNode(const char* target_address, +SubchannelNode::SubchannelNode(std::string target_address, size_t channel_tracer_max_nodes) - : BaseNode(EntityType::kSubchannel, - UniquePtr(gpr_strdup(target_address))), - target_(UniquePtr(gpr_strdup(target_address))), + : BaseNode(EntityType::kSubchannel, target_address), + target_(std::move(target_address)), trace_(channel_tracer_max_nodes) {} SubchannelNode::~SubchannelNode() {} @@ -76,8 +75,8 @@ grpc_json* SubchannelNode::RenderJson() { json = data; json_iterator = nullptr; PopulateConnectivityState(json); - GPR_ASSERT(target_.get() != nullptr); - grpc_json_create_child(nullptr, json, "target", target_.get(), + GPR_ASSERT(!target_.empty()); + grpc_json_create_child(nullptr, json, "target", target_.c_str(), GRPC_JSON_STRING, false); // fill in the channel trace if applicable grpc_json* trace_json = trace_.RenderJson(); @@ -102,7 +101,8 @@ grpc_json* SubchannelNode::RenderJson() { grpc_json* sibling_iterator = grpc_json_add_number_string_child( json_iterator, nullptr, "socketId", child_socket->uuid()); grpc_json_create_child(sibling_iterator, json_iterator, "name", - child_socket->name(), GRPC_JSON_STRING, false); + child_socket->name().c_str(), GRPC_JSON_STRING, + false); } return top_level_json; } diff --git a/src/core/ext/filters/client_channel/client_channel_channelz.h b/src/core/ext/filters/client_channel/client_channel_channelz.h index 5a1a1adcd1b..270aa543dfc 100644 --- a/src/core/ext/filters/client_channel/client_channel_channelz.h +++ b/src/core/ext/filters/client_channel/client_channel_channelz.h @@ -21,6 +21,8 @@ #include +#include + #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/channel/channel_stack.h" #include "src/core/lib/channel/channel_trace.h" @@ -34,7 +36,7 @@ namespace channelz { class SubchannelNode : public BaseNode { public: - SubchannelNode(const char* target_address, size_t channel_tracer_max_nodes); + SubchannelNode(std::string target_address, size_t channel_tracer_max_nodes); ~SubchannelNode() override; // Sets the subchannel's connectivity state without health checking. @@ -67,7 +69,7 @@ class SubchannelNode : public BaseNode { Atomic connectivity_state_{GRPC_CHANNEL_IDLE}; Mutex socket_mu_; RefCountedPtr child_socket_; - UniquePtr target_; + std::string target_; CallCountingHelper call_counter_; ChannelTrace trace_; }; diff --git a/src/core/ext/transport/chttp2/server/chttp2_server.cc b/src/core/ext/transport/chttp2/server/chttp2_server.cc index f37551fce2d..d7e15959b23 100644 --- a/src/core/ext/transport/chttp2/server/chttp2_server.cc +++ b/src/core/ext/transport/chttp2/server/chttp2_server.cc @@ -417,8 +417,10 @@ grpc_error* grpc_chttp2_server_add_port(grpc_server* server, const char* addr, gpr_asprintf(&socket_name, "chttp2 listener %s", addr); state->channelz_listen_socket = grpc_core::MakeRefCounted( - grpc_core::UniquePtr(gpr_strdup(addr)), - grpc_core::UniquePtr(socket_name)); + addr, socket_name); + // TODO(veblush): Remove this once gpr_asprintf is replaced by + // absl::StrFormat + gpr_free(socket_name); } /* Register with the server only upon success */ diff --git a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc index e0aa8e5d9c8..db68eda7484 100644 --- a/src/core/ext/transport/chttp2/transport/chttp2_transport.cc +++ b/src/core/ext/transport/chttp2/transport/chttp2_transport.cc @@ -382,9 +382,10 @@ static bool read_channel_args(grpc_chttp2_transport* t, gpr_asprintf(&socket_name, "%s %s", get_vtable()->name, t->peer_string); t->channelz_socket = grpc_core::MakeRefCounted( - grpc_core::UniquePtr(), - grpc_core::UniquePtr(gpr_strdup(t->peer_string)), - grpc_core::UniquePtr(socket_name)); + "", t->peer_string, socket_name); + // TODO(veblush): Remove this once gpr_asprintf is replaced by + // absl::StrFormat + gpr_free(socket_name); } return enable_bdp; } diff --git a/src/core/lib/channel/channelz.cc b/src/core/lib/channel/channelz.cc index 6300c230d4e..f5d06c78a80 100644 --- a/src/core/lib/channel/channelz.cc +++ b/src/core/lib/channel/channelz.cc @@ -85,7 +85,7 @@ intptr_t GetParentUuidFromArgs(const grpc_channel_args& args) { // BaseNode // -BaseNode::BaseNode(EntityType type, UniquePtr name) +BaseNode::BaseNode(EntityType type, std::string name) : type_(type), uuid_(-1), name_(std::move(name)) { // The registry will set uuid_ under its lock. ChannelzRegistry::Register(this); @@ -180,11 +180,11 @@ void CallCountingHelper::PopulateCallCounts(grpc_json* json) { // ChannelNode // -ChannelNode::ChannelNode(UniquePtr target, - size_t channel_tracer_max_nodes, intptr_t parent_uuid) +ChannelNode::ChannelNode(std::string target, size_t channel_tracer_max_nodes, + intptr_t parent_uuid) : BaseNode(parent_uuid == 0 ? EntityType::kTopLevelChannel : EntityType::kInternalChannel, - UniquePtr(gpr_strdup(target.get()))), + target), target_(std::move(target)), trace_(channel_tracer_max_nodes), parent_uuid_(parent_uuid) {} @@ -239,8 +239,8 @@ grpc_json* ChannelNode::RenderJson() { json = data; } // populate the target. - GPR_ASSERT(target_.get() != nullptr); - grpc_json_create_child(nullptr, json, "target", target_.get(), + GPR_ASSERT(!target_.empty()); + grpc_json_create_child(nullptr, json, "target", target_.c_str(), GRPC_JSON_STRING, false); // fill in the channel trace if applicable grpc_json* trace_json = trace_.RenderJson(); @@ -316,8 +316,7 @@ void ChannelNode::RemoveChildSubchannel(intptr_t child_uuid) { // ServerNode::ServerNode(grpc_server* /*server*/, size_t channel_tracer_max_nodes) - : BaseNode(EntityType::kServer, /* name */ nullptr), - trace_(channel_tracer_max_nodes) {} + : BaseNode(EntityType::kServer, ""), trace_(channel_tracer_max_nodes) {} ServerNode::~ServerNode() {} @@ -363,7 +362,8 @@ char* ServerNode::RenderServerSockets(intptr_t start_socket_id, json_iterator = grpc_json_add_number_string_child( socket_ref_json, nullptr, "socketId", it->first); grpc_json_create_child(json_iterator, socket_ref_json, "name", - it->second->name(), GRPC_JSON_STRING, false); + it->second->name().c_str(), GRPC_JSON_STRING, + false); } } if (sockets_rendered == child_sockets_.size()) { @@ -416,7 +416,8 @@ grpc_json* ServerNode::RenderJson() { grpc_json* sibling_iterator = grpc_json_add_number_string_child( json_iterator, nullptr, "socketId", it.first); grpc_json_create_child(sibling_iterator, json_iterator, "name", - it.second->name(), GRPC_JSON_STRING, false); + it.second->name().c_str(), GRPC_JSON_STRING, + false); } } return top_level_json; @@ -479,8 +480,7 @@ void PopulateSocketAddressJson(grpc_json* json, const char* name, } // namespace -SocketNode::SocketNode(UniquePtr local, UniquePtr remote, - UniquePtr name) +SocketNode::SocketNode(std::string local, std::string remote, std::string name) : BaseNode(EntityType::kSocket, std::move(name)), local_(std::move(local)), remote_(std::move(remote)) {} @@ -520,11 +520,11 @@ grpc_json* SocketNode::RenderJson() { json_iterator = nullptr; json_iterator = grpc_json_add_number_string_child(json, json_iterator, "socketId", uuid()); - json_iterator = grpc_json_create_child(json_iterator, json, "name", name(), - GRPC_JSON_STRING, false); + json_iterator = grpc_json_create_child( + json_iterator, json, "name", name().c_str(), GRPC_JSON_STRING, false); json = top_level_json; - PopulateSocketAddressJson(json, "remote", remote_.get()); - PopulateSocketAddressJson(json, "local", local_.get()); + PopulateSocketAddressJson(json, "remote", remote_.c_str()); + PopulateSocketAddressJson(json, "local", local_.c_str()); // reset json iterators to top level object json = top_level_json; json_iterator = nullptr; @@ -605,8 +605,7 @@ grpc_json* SocketNode::RenderJson() { // ListenSocketNode // -ListenSocketNode::ListenSocketNode(UniquePtr local_addr, - UniquePtr name) +ListenSocketNode::ListenSocketNode(std::string local_addr, std::string name) : BaseNode(EntityType::kSocket, std::move(name)), local_addr_(std::move(local_addr)) {} @@ -622,10 +621,10 @@ grpc_json* ListenSocketNode::RenderJson() { json_iterator = nullptr; json_iterator = grpc_json_add_number_string_child(json, json_iterator, "socketId", uuid()); - json_iterator = grpc_json_create_child(json_iterator, json, "name", name(), - GRPC_JSON_STRING, false); + json_iterator = grpc_json_create_child( + json_iterator, json, "name", name().c_str(), GRPC_JSON_STRING, false); json = top_level_json; - PopulateSocketAddressJson(json, "local", local_addr_.get()); + PopulateSocketAddressJson(json, "local", local_addr_.c_str()); return top_level_json; } diff --git a/src/core/lib/channel/channelz.h b/src/core/lib/channel/channelz.h index 5a063b4df96..8702cbe9e95 100644 --- a/src/core/lib/channel/channelz.h +++ b/src/core/lib/channel/channelz.h @@ -23,6 +23,8 @@ #include +#include + #include "src/core/lib/channel/channel_trace.h" #include "src/core/lib/gpr/time_precise.h" #include "src/core/lib/gprpp/inlined_vector.h" @@ -82,7 +84,7 @@ class BaseNode : public RefCounted { }; protected: - BaseNode(EntityType type, UniquePtr name); + BaseNode(EntityType type, std::string name); public: virtual ~BaseNode(); @@ -96,14 +98,14 @@ class BaseNode : public RefCounted { EntityType type() const { return type_; } intptr_t uuid() const { return uuid_; } - const char* name() const { return name_.get(); } + const std::string& name() const { return name_; } private: // to allow the ChannelzRegistry to set uuid_ under its lock. friend class ChannelzRegistry; const EntityType type_; intptr_t uuid_; - UniquePtr name_; + std::string name_; }; // This class is a helper class for channelz entities that deal with Channels, @@ -165,7 +167,7 @@ class CallCountingHelper { // Handles channelz bookkeeping for channels class ChannelNode : public BaseNode { public: - ChannelNode(UniquePtr target, size_t channel_tracer_max_nodes, + ChannelNode(std::string target, size_t channel_tracer_max_nodes, intptr_t parent_uuid); // Returns the string description of the given connectivity state. @@ -208,7 +210,7 @@ class ChannelNode : public BaseNode { // to allow the channel trace test to access trace_. friend class testing::ChannelNodePeer; - UniquePtr target_; + std::string target_; CallCountingHelper call_counter_; ChannelTrace trace_; const intptr_t parent_uuid_; @@ -269,8 +271,7 @@ class ServerNode : public BaseNode { // Handles channelz bookkeeping for sockets class SocketNode : public BaseNode { public: - SocketNode(UniquePtr local, UniquePtr remote, - UniquePtr name); + SocketNode(std::string local, std::string remote, std::string name); ~SocketNode() override {} grpc_json* RenderJson() override; @@ -289,7 +290,7 @@ class SocketNode : public BaseNode { gpr_atm_no_barrier_fetch_add(&keepalives_sent_, static_cast(1)); } - const char* remote() { return remote_.get(); } + const std::string& remote() { return remote_; } private: gpr_atm streams_started_ = 0; @@ -302,20 +303,20 @@ class SocketNode : public BaseNode { gpr_atm last_remote_stream_created_cycle_ = 0; gpr_atm last_message_sent_cycle_ = 0; gpr_atm last_message_received_cycle_ = 0; - UniquePtr local_; - UniquePtr remote_; + std::string local_; + std::string remote_; }; // Handles channelz bookkeeping for listen sockets class ListenSocketNode : public BaseNode { public: - ListenSocketNode(UniquePtr local_addr, UniquePtr name); + ListenSocketNode(std::string local_addr, std::string name); ~ListenSocketNode() override {} grpc_json* RenderJson() override; private: - UniquePtr local_addr_; + std::string local_addr_; }; } // namespace channelz diff --git a/src/core/lib/gprpp/string_view.h b/src/core/lib/gprpp/string_view.h index 7823b930709..e2e082e3ea5 100644 --- a/src/core/lib/gprpp/string_view.h +++ b/src/core/lib/gprpp/string_view.h @@ -28,6 +28,7 @@ #include #include #include +#include #include "src/core/lib/gpr/string.h" #include "src/core/lib/gpr/useful.h" @@ -111,6 +112,15 @@ class StringView final { size_ = 0; } + // Converts to `std::basic_string`. + template + explicit operator std::basic_string, Allocator>() + const { + if (data() == nullptr) return {}; + return std::basic_string, Allocator>(data(), + size()); + } + private: const char* ptr_; size_t size_; diff --git a/src/core/lib/surface/channel.cc b/src/core/lib/surface/channel.cc index afc4042a2ba..9f5abca9c47 100644 --- a/src/core/lib/surface/channel.cc +++ b/src/core/lib/surface/channel.cc @@ -202,11 +202,11 @@ void CreateChannelzNode(grpc_channel_stack_builder* builder) { const intptr_t channelz_parent_uuid = grpc_core::channelz::GetParentUuidFromArgs(*args); // Create the channelz node. + const char* target = grpc_channel_stack_builder_get_target(builder); grpc_core::RefCountedPtr channelz_node = grpc_core::MakeRefCounted( - grpc_core::UniquePtr( - gpr_strdup(grpc_channel_stack_builder_get_target(builder))), - channel_tracer_max_memory, channelz_parent_uuid); + target != nullptr ? target : "", channel_tracer_max_memory, + channelz_parent_uuid); channelz_node->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, grpc_slice_from_static_string("Channel created")); diff --git a/test/core/channel/channel_trace_test.cc b/test/core/channel/channel_trace_test.cc index 5ac7f0871a4..a957dca0f97 100644 --- a/test/core/channel/channel_trace_test.cc +++ b/test/core/channel/channel_trace_test.cc @@ -174,8 +174,8 @@ TEST(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); AddSimpleTrace(&tracer); ChannelFixture channel1(kEventListMemoryLimit); - RefCountedPtr sc1 = MakeRefCounted( - UniquePtr(gpr_strdup("fake_target")), kEventListMemoryLimit, 0); + RefCountedPtr sc1 = + MakeRefCounted("fake_target", kEventListMemoryLimit, 0); ChannelNodePeer sc1_peer(sc1.get()); tracer.AddTraceEventWithReference( ChannelTrace::Severity::Info, @@ -193,8 +193,8 @@ TEST(ChannelTracerTest, ComplexTest) { AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 5); ChannelFixture channel2(kEventListMemoryLimit); - RefCountedPtr sc2 = MakeRefCounted( - UniquePtr(gpr_strdup("fake_target")), kEventListMemoryLimit, 0); + RefCountedPtr sc2 = + MakeRefCounted("fake_target", kEventListMemoryLimit, 0); tracer.AddTraceEventWithReference( ChannelTrace::Severity::Info, grpc_slice_from_static_string("LB channel two created"), sc2); @@ -222,8 +222,8 @@ TEST(ChannelTracerTest, TestNesting) { AddSimpleTrace(&tracer); ValidateChannelTrace(&tracer, 2); ChannelFixture channel1(kEventListMemoryLimit); - RefCountedPtr sc1 = MakeRefCounted( - UniquePtr(gpr_strdup("fake_target")), kEventListMemoryLimit, 0); + RefCountedPtr sc1 = + MakeRefCounted("fake_target", kEventListMemoryLimit, 0); ChannelNodePeer sc1_peer(sc1.get()); tracer.AddTraceEventWithReference( ChannelTrace::Severity::Info, @@ -231,8 +231,8 @@ TEST(ChannelTracerTest, TestNesting) { ValidateChannelTrace(&tracer, 3); AddSimpleTrace(sc1_peer.trace()); ChannelFixture channel2(kEventListMemoryLimit); - RefCountedPtr conn1 = MakeRefCounted( - UniquePtr(gpr_strdup("fake_target")), kEventListMemoryLimit, 0); + RefCountedPtr conn1 = + MakeRefCounted("fake_target", kEventListMemoryLimit, 0); ChannelNodePeer conn1_peer(conn1.get()); // nesting one level deeper. sc1_peer.trace()->AddTraceEventWithReference( @@ -245,8 +245,8 @@ TEST(ChannelTracerTest, TestNesting) { ValidateChannelTrace(&tracer, 5); ValidateChannelTrace(conn1_peer.trace(), 1); ChannelFixture channel3(kEventListMemoryLimit); - RefCountedPtr sc2 = MakeRefCounted( - UniquePtr(gpr_strdup("fake_target")), kEventListMemoryLimit, 0); + RefCountedPtr sc2 = + MakeRefCounted("fake_target", kEventListMemoryLimit, 0); tracer.AddTraceEventWithReference( ChannelTrace::Severity::Info, grpc_slice_from_static_string("subchannel two created"), sc2); diff --git a/test/core/channel/channelz_registry_test.cc b/test/core/channel/channelz_registry_test.cc index 39d3b1d5eeb..eca7f20ead6 100644 --- a/test/core/channel/channelz_registry_test.cc +++ b/test/core/channel/channelz_registry_test.cc @@ -52,8 +52,7 @@ class ChannelzRegistryTest : public ::testing::Test { }; static RefCountedPtr CreateTestNode() { - return MakeRefCounted(UniquePtr(gpr_strdup("test")), - UniquePtr(gpr_strdup("test"))); + return MakeRefCounted("test", "test"); } TEST_F(ChannelzRegistryTest, UuidStartsAboveZeroTest) {