From 7190b0c6510b75766362c43ec32c9e497057bced Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 26 Oct 2016 09:02:37 -0700 Subject: [PATCH 01/28] Add ChannelArguments::SetLoadBalancingPolicyName() to C++ API. --- include/grpc++/support/channel_arguments.h | 5 +++++ src/cpp/common/channel_arguments.cc | 5 +++++ 2 files changed, 10 insertions(+) diff --git a/include/grpc++/support/channel_arguments.h b/include/grpc++/support/channel_arguments.h index ae243939e9a..9de5497fb8f 100644 --- a/include/grpc++/support/channel_arguments.h +++ b/include/grpc++/support/channel_arguments.h @@ -80,6 +80,11 @@ class ChannelArguments { /// The given string will be sent at the front of the user agent string. void SetUserAgentPrefix(const grpc::string& user_agent_prefix); + // Set LB policy name. + // Note that if the name resolver returns only balancer addresses, the + // grpclb LB policy will be used, regardless of what is specified here. + void SetLoadBalancingPolicyName(const grpc::string& lb_policy_name); + // Generic channel argument setters. Only for advanced use cases. /// Set an integer argument \a value under \a key. void SetInt(const grpc::string& key, int value); diff --git a/src/cpp/common/channel_arguments.cc b/src/cpp/common/channel_arguments.cc index f297ae85870..d16d27e86cf 100644 --- a/src/cpp/common/channel_arguments.cc +++ b/src/cpp/common/channel_arguments.cc @@ -113,6 +113,11 @@ void ChannelArguments::SetUserAgentPrefix( } } +void ChannelArguments::SetLoadBalancingPolicyName( + const grpc::string& lb_policy_name) { + SetString(GRPC_ARG_LB_POLICY_NAME, lb_policy_name); +} + void ChannelArguments::SetInt(const grpc::string& key, int value) { grpc_arg arg; arg.type = GRPC_ARG_INTEGER; From 452287607fd2d838e1f5dd7f009cbf7ad95e2346 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 26 Oct 2016 14:54:56 -0700 Subject: [PATCH 02/28] Add test for setting LB policy via C++ API. --- Makefile | 48 ++++ build.yaml | 13 ++ test/cpp/end2end/round_robin_end2end_test.cc | 209 ++++++++++++++++++ tools/run_tests/sources_and_headers.json | 19 ++ tools/run_tests/tests.json | 22 ++ .../round_robin_end2end_test.vcxproj | 207 +++++++++++++++++ .../round_robin_end2end_test.vcxproj.filters | 21 ++ 7 files changed, 539 insertions(+) create mode 100644 test/cpp/end2end/round_robin_end2end_test.cc create mode 100644 vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj create mode 100644 vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj.filters diff --git a/Makefile b/Makefile index e789b1f38b6..e13c0045092 100644 --- a/Makefile +++ b/Makefile @@ -1075,6 +1075,7 @@ qps_openloop_test: $(BINDIR)/$(CONFIG)/qps_openloop_test qps_worker: $(BINDIR)/$(CONFIG)/qps_worker reconnect_interop_client: $(BINDIR)/$(CONFIG)/reconnect_interop_client reconnect_interop_server: $(BINDIR)/$(CONFIG)/reconnect_interop_server +round_robin_end2end_test: $(BINDIR)/$(CONFIG)/round_robin_end2end_test secure_auth_context_test: $(BINDIR)/$(CONFIG)/secure_auth_context_test secure_sync_unary_ping_pong_test: $(BINDIR)/$(CONFIG)/secure_sync_unary_ping_pong_test server_builder_plugin_test: $(BINDIR)/$(CONFIG)/server_builder_plugin_test @@ -1451,6 +1452,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/qps_worker \ $(BINDIR)/$(CONFIG)/reconnect_interop_client \ $(BINDIR)/$(CONFIG)/reconnect_interop_server \ + $(BINDIR)/$(CONFIG)/round_robin_end2end_test \ $(BINDIR)/$(CONFIG)/secure_auth_context_test \ $(BINDIR)/$(CONFIG)/secure_sync_unary_ping_pong_test \ $(BINDIR)/$(CONFIG)/server_builder_plugin_test \ @@ -1539,6 +1541,7 @@ buildtests_cxx: privatelibs_cxx \ $(BINDIR)/$(CONFIG)/qps_worker \ $(BINDIR)/$(CONFIG)/reconnect_interop_client \ $(BINDIR)/$(CONFIG)/reconnect_interop_server \ + $(BINDIR)/$(CONFIG)/round_robin_end2end_test \ $(BINDIR)/$(CONFIG)/secure_auth_context_test \ $(BINDIR)/$(CONFIG)/secure_sync_unary_ping_pong_test \ $(BINDIR)/$(CONFIG)/server_builder_plugin_test \ @@ -1845,6 +1848,8 @@ test_cxx: buildtests_cxx $(Q) $(BINDIR)/$(CONFIG)/proto_server_reflection_test || ( echo test proto_server_reflection_test failed ; exit 1 ) $(E) "[RUN] Testing qps_openloop_test" $(Q) $(BINDIR)/$(CONFIG)/qps_openloop_test || ( echo test qps_openloop_test failed ; exit 1 ) + $(E) "[RUN] Testing round_robin_end2end_test" + $(Q) $(BINDIR)/$(CONFIG)/round_robin_end2end_test || ( echo test round_robin_end2end_test failed ; exit 1 ) $(E) "[RUN] Testing secure_auth_context_test" $(Q) $(BINDIR)/$(CONFIG)/secure_auth_context_test || ( echo test secure_auth_context_test failed ; exit 1 ) $(E) "[RUN] Testing secure_sync_unary_ping_pong_test" @@ -13014,6 +13019,49 @@ endif $(OBJDIR)/$(CONFIG)/test/cpp/interop/reconnect_interop_server.o: $(GENDIR)/src/proto/grpc/testing/empty.pb.cc $(GENDIR)/src/proto/grpc/testing/empty.grpc.pb.cc $(GENDIR)/src/proto/grpc/testing/messages.pb.cc $(GENDIR)/src/proto/grpc/testing/messages.grpc.pb.cc $(GENDIR)/src/proto/grpc/testing/test.pb.cc $(GENDIR)/src/proto/grpc/testing/test.grpc.pb.cc +ROUND_ROBIN_END2END_TEST_SRC = \ + test/cpp/end2end/round_robin_end2end_test.cc \ + +ROUND_ROBIN_END2END_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(ROUND_ROBIN_END2END_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/round_robin_end2end_test: openssl_dep_error + +else + + + + +ifeq ($(NO_PROTOBUF),true) + +# You can't build the protoc plugins or protobuf-enabled targets if you don't have protobuf 3.0.0+. + +$(BINDIR)/$(CONFIG)/round_robin_end2end_test: protobuf_dep_error + +else + +$(BINDIR)/$(CONFIG)/round_robin_end2end_test: $(PROTOBUF_DEP) $(ROUND_ROBIN_END2END_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LDXX) $(LDFLAGS) $(ROUND_ROBIN_END2END_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBSXX) $(LDLIBS_PROTOBUF) $(LDLIBS) $(LDLIBS_SECURE) $(GTEST_LIB) -o $(BINDIR)/$(CONFIG)/round_robin_end2end_test + +endif + +endif + +$(OBJDIR)/$(CONFIG)/test/cpp/end2end/round_robin_end2end_test.o: $(LIBDIR)/$(CONFIG)/libgrpc++_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc++.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_round_robin_end2end_test: $(ROUND_ROBIN_END2END_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(ROUND_ROBIN_END2END_TEST_OBJS:.o=.dep) +endif +endif + + SECURE_AUTH_CONTEXT_TEST_SRC = \ test/cpp/common/secure_auth_context_test.cc \ diff --git a/build.yaml b/build.yaml index 0400fa7b213..bd2349b8ef4 100644 --- a/build.yaml +++ b/build.yaml @@ -3319,6 +3319,19 @@ targets: - gpr_test_util - gpr - grpc++_test_config +- name: round_robin_end2end_test + gtest: true + build: test + language: c++ + src: + - test/cpp/end2end/round_robin_end2end_test.cc + deps: + - grpc++_test_util + - grpc_test_util + - grpc++ + - grpc + - gpr_test_util + - gpr - name: secure_auth_context_test gtest: true build: test diff --git a/test/cpp/end2end/round_robin_end2end_test.cc b/test/cpp/end2end/round_robin_end2end_test.cc new file mode 100644 index 00000000000..fe0cd0baa40 --- /dev/null +++ b/test/cpp/end2end/round_robin_end2end_test.cc @@ -0,0 +1,209 @@ +/* + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ + +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include "src/proto/grpc/testing/echo.grpc.pb.h" +#include "test/core/util/port.h" +#include "test/core/util/test_config.h" +#include "test/cpp/end2end/test_service_impl.h" + +using grpc::testing::EchoRequest; +using grpc::testing::EchoResponse; +using std::chrono::system_clock; + +namespace grpc { +namespace testing { +namespace { + +// Subclass of TestServiceImpl that increments a request counter for +// every call to the Echo RPC. +class MyTestServiceImpl : public TestServiceImpl { + public: + Status Echo(ServerContext* context, const EchoRequest* request, + EchoResponse* response) GRPC_OVERRIDE { + { + unique_lock lock(mu_); + ++request_count_; + } + return TestServiceImpl::Echo(context, request, response); + } + + int request_count() { + unique_lock lock(mu_); + return request_count_; + } + + private: + mutex mu_; + int request_count_ = 0; +}; + +class RoundRobinEnd2endTest : public ::testing::Test { + protected: + RoundRobinEnd2endTest() : server_host_("localhost") {} + + void StartServers(int num_servers) { + for (int i = 0; i < num_servers; ++i) { + servers_.emplace_back(new ServerData(server_host_)); + } + } + + void TearDown() GRPC_OVERRIDE { + for (const auto& server : servers_) { + server->Shutdown(); + } + } + + void ResetStub(bool round_robin) { + ChannelArguments args; + if (round_robin) args.SetLoadBalancingPolicyName("round_robin"); + std::ostringstream uri; + uri << "ipv4:///"; + for (size_t i = 0; i < servers_.size() - 1; ++i) { + uri << "127.0.0.1:" << servers_[i]->port_ << ","; + } + uri << "127.0.0.1:" << servers_[servers_.size() - 1]->port_; + std::shared_ptr channel = CreateCustomChannel( + uri.str(), InsecureChannelCredentials(), args); + stub_ = grpc::testing::EchoTestService::NewStub(channel); + } + + void SendRpc(int num_rpcs) { + EchoRequest request; + EchoResponse response; + request.set_message("Live long and prosper."); + for (int i = 0; i < num_rpcs; i++) { + ClientContext context; + Status status = stub_->Echo(&context, request, &response); + EXPECT_TRUE(status.ok()); + EXPECT_EQ(response.message(), request.message()); + } + } + + struct ServerData { + int port_; + std::unique_ptr server_; + MyTestServiceImpl service_; + std::thread* thread_; + + explicit ServerData(const grpc::string& server_host) { + port_ = grpc_pick_unused_port_or_die(); + gpr_log(GPR_INFO, "starting server on port %d", port_); + std::mutex mu; + std::condition_variable cond; + thread_ = new std::thread([this, server_host, &mu, &cond]() { + Start(server_host); + lock_guard lock(mu); + cond.notify_one(); + }); + unique_lock lock(mu); + cond.wait(lock); + gpr_log(GPR_INFO, "server startup complete"); + } + + void Start(const grpc::string& server_host) { + std::ostringstream server_address; + server_address << server_host << ":" << port_; + ServerBuilder builder; + builder.AddListeningPort(server_address.str(), + InsecureServerCredentials()); + builder.RegisterService(&service_); + server_ = builder.BuildAndStart(); + } + + void Shutdown() { + server_->Shutdown(); + thread_->join(); + } + }; + + const grpc::string server_host_; + CompletionQueue cli_cq_; + std::unique_ptr stub_; + std::vector> servers_; +}; + +TEST_F(RoundRobinEnd2endTest, PickFirst) { + // Start servers and send one RPC per server. + const int kNumServers = 3; + StartServers(kNumServers); + ResetStub(false /* round_robin */); + SendRpc(kNumServers); + // All requests should have gone to a single server. + bool found = false; + for (const auto& server : servers_) { + const int request_count = server->service_.request_count(); + if (request_count == kNumServers) { + found = true; + } else { + EXPECT_EQ(0, request_count); + } + } + EXPECT_TRUE(found); +} + +TEST_F(RoundRobinEnd2endTest, RoundRobin) { + // Start servers and send one RPC per server. + const int kNumServers = 3; + StartServers(kNumServers); + ResetStub(true /* round_robin */); + SendRpc(kNumServers); + // One request should have gone to each server. + for (const auto& server : servers_) { + EXPECT_EQ(1, server->service_.request_count()); + } +} + +} // namespace +} // namespace testing +} // namespace grpc + +int main(int argc, char** argv) { + grpc_test_init(argc, argv); + ::testing::InitGoogleTest(&argc, argv); + return RUN_ALL_TESTS(); +} diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 46a83c24794..4b5ba6a1d5a 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -2971,6 +2971,25 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc++", + "grpc++_test_util", + "grpc_test_util" + ], + "headers": [], + "is_filegroup": false, + "language": "c++", + "name": "round_robin_end2end_test", + "src": [ + "test/cpp/end2end/round_robin_end2end_test.cc" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 5cb50cc999c..278e2868519 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -2801,6 +2801,28 @@ "posix" ] }, + { + "args": [], + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "exclude_iomgrs": [], + "flaky": false, + "gtest": true, + "language": "c++", + "name": "round_robin_end2end_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ] + }, { "args": [], "ci_platforms": [ diff --git a/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj b/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj new file mode 100644 index 00000000000..55e16f188dc --- /dev/null +++ b/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj @@ -0,0 +1,207 @@ + + + + + + Debug + Win32 + + + Debug + x64 + + + Release + Win32 + + + Release + x64 + + + + {54B15DF6-42BA-5347-C9B8-2D7F1F2921C6} + true + $(SolutionDir)IntDir\$(MSBuildProjectName)\ + + + + v100 + + + v110 + + + v120 + + + v140 + + + Application + true + Unicode + + + Application + false + true + Unicode + + + + + + + + + + + + + + + + round_robin_end2end_test + static + Debug + static + Debug + + + round_robin_end2end_test + static + Release + static + Release + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + Disabled + WIN32;_DEBUG;_LIB;%(PreprocessorDefinitions) + true + MultiThreadedDebug + true + None + false + + + Console + true + false + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + NotUsing + Level3 + MaxSpeed + WIN32;NDEBUG;_LIB;%(PreprocessorDefinitions) + true + true + true + MultiThreaded + true + None + false + + + Console + true + false + true + true + + + + + + + + + + {0BE77741-552A-929B-A497-4EF7ECE17A64} + + + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + + + {C187A093-A0FE-489D-A40A-6E33DE0F9FEB} + + + {29D16885-7228-4C31-81ED-5F9187C7F2A9} + + + {EAB0A629-17A9-44DB-B5FF-E91A721FE037} + + + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + + + + + + + + + + + + + + + This project references NuGet package(s) that are missing on this computer. Enable NuGet Package Restore to download them. For more information, see http://go.microsoft.com/fwlink/?LinkID=322105. The missing file is {0}. + + + + + + + + + diff --git a/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj.filters b/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj.filters new file mode 100644 index 00000000000..95a149953fc --- /dev/null +++ b/vsprojects/vcxproj/test/round_robin_end2end_test/round_robin_end2end_test.vcxproj.filters @@ -0,0 +1,21 @@ + + + + + test\cpp\end2end + + + + + + {e151f47d-6563-5ef9-fae6-70708f9f8ee6} + + + {07958594-fd93-28f7-9388-c67c952701b8} + + + {7596a0dd-caa4-b365-a59f-f7ffef38b10d} + + + + From 0baf1dc569a112bc71039b84bdaef0bad68cf852 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 28 Oct 2016 04:44:01 +0200 Subject: [PATCH 03/28] Made LB token dynamic size <= 50 bytes --- src/core/ext/lb_policy/grpclb/grpclb.c | 8 +++++--- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 4 ++-- src/proto/grpc/lb/v1/load_balancer.options | 2 +- test/cpp/grpclb/grpclb_test.cc | 2 -- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 6da4febf266..5bbf857079f 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -407,10 +407,12 @@ static grpc_lb_addresses *process_serverlist( /* lb token processing */ void *user_data; if (server->has_load_balance_token) { - const size_t lb_token_size = - GPR_ARRAY_SIZE(server->load_balance_token) - 1; + const size_t lb_token_max_length = + GPR_ARRAY_SIZE(server->load_balance_token); + const size_t lb_token_length = + strnlen(server->load_balance_token, lb_token_max_length); grpc_mdstr *lb_token_mdstr = grpc_mdstr_from_buffer( - (uint8_t *)server->load_balance_token, lb_token_size); + (uint8_t *)server->load_balance_token, lb_token_length); user_data = grpc_mdelem_from_metadata_strings(GRPC_MDSTR_LB_TOKEN, lb_token_mdstr); } else { diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index 53fed22bae0..e36d0966f85 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -77,7 +77,7 @@ typedef struct _grpc_lb_v1_Server { bool has_port; int32_t port; bool has_load_balance_token; - char load_balance_token[65]; + char load_balance_token[50]; bool has_drop_request; bool drop_request; /* @@protoc_insertion_point(struct:grpc_lb_v1_Server) */ @@ -172,7 +172,7 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 /* grpc_lb_v1_ServerList_size depends on runtime parameters */ -#define grpc_lb_v1_Server_size 98 +#define grpc_lb_v1_Server_size 83 /* Message IDs (where set with "msgid" option) */ #ifdef PB_MSGID diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index a9398d5f474..f4afc1ee61d 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -2,5 +2,5 @@ grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 grpc.lb.v1.InitialLoadBalanceResponse.client_config max_size:64 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 -grpc.lb.v1.Server.load_balance_token max_size:65 +grpc.lb.v1.Server.load_balance_token max_size:50 load_balancer.proto no_unions:true diff --git a/test/cpp/grpclb/grpclb_test.cc b/test/cpp/grpclb/grpclb_test.cc index b6056f9ae4c..d7882404381 100644 --- a/test/cpp/grpclb/grpclb_test.cc +++ b/test/cpp/grpclb/grpclb_test.cc @@ -144,7 +144,6 @@ static gpr_slice build_response_payload_slice( // disfunctional implementation of std::to_string in gcc 4.4, which doesn't // have a version for int but does have one for long long int. string token_data = "token" + std::to_string((long long int)ports[i]); - token_data.resize(64, '-'); server->set_load_balance_token(token_data); } const grpc::string &enc_resp = response.SerializeAsString(); @@ -333,7 +332,6 @@ static void start_backend_server(server_fixture *sf) { // disfunctional implementation of std::to_string in gcc 4.4, which doesn't // have a version for int but does have one for long long int. string expected_token = "token" + std::to_string((long long int)sf->port); - expected_token.resize(64, '-'); GPR_ASSERT(contains_metadata(&request_metadata_recv, "lb-token", expected_token.c_str())); From 472fc2ffd10026ff6f61886c5a8c26863aae966b Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 28 Oct 2016 21:01:26 +0200 Subject: [PATCH 04/28] Updated proto field comment with max token length --- src/proto/grpc/lb/v1/load_balancer.proto | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index 210fba1323e..b6849c10d7d 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -130,6 +130,8 @@ message Server { // frontend requests for that pick must include the token in its initial // metadata. The token is used by the backend to verify the request and to // allow the backend to report load to the gRPC LB system. + // + // Its length is variable but less than 50 bytes. string load_balance_token = 3; // Indicates whether this particular request should be dropped by the client From b928de86c769ab9f368d65001ad6ca30dd50844d Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Fri, 28 Oct 2016 15:05:33 -0700 Subject: [PATCH 05/28] Set the long_description setup.py field Somehow ad hoc descriptions don't work anymore for grpcio (probably because the README.rst isn't at the distribution root for grpcio). --- setup.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/setup.py b/setup.py index cdd3bb3f0dd..559a75f6742 100644 --- a/setup.py +++ b/setup.py @@ -52,6 +52,7 @@ PYTHON_STEM = os.path.join('src', 'python', 'grpcio') CORE_INCLUDE = ('include', '.',) BORINGSSL_INCLUDE = (os.path.join('third_party', 'boringssl', 'include'),) ZLIB_INCLUDE = (os.path.join('third_party', 'zlib'),) +README = os.path.join(PYTHON_STEM, 'README.rst') # Ensure we're in the proper directory whether or not we're being used by pip. os.chdir(os.path.dirname(os.path.abspath(__file__))) @@ -259,6 +260,7 @@ setuptools.setup( name='grpcio', version=grpc_version.VERSION, license=LICENSE, + long_description=open(README).read(), ext_modules=CYTHON_EXTENSION_MODULES, packages=list(PACKAGES), package_dir=PACKAGE_DIRECTORIES, From 2a23c60a4b7734623f3c1ae49478d182a25b6421 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sat, 29 Oct 2016 01:26:17 +0200 Subject: [PATCH 06/28] Removed spurious option for lb nanopb proto --- src/proto/grpc/lb/v1/load_balancer.options | 1 - 1 file changed, 1 deletion(-) diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index f4afc1ee61d..7fbd44b9ded 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,5 +1,4 @@ grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 -grpc.lb.v1.InitialLoadBalanceResponse.client_config max_size:64 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 From 41752d6e7bb3b414139e854678bae2a962f0d82e Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sat, 29 Oct 2016 01:26:30 +0200 Subject: [PATCH 07/28] Added max length comments for some lb proto fields --- src/proto/grpc/lb/v1/load_balancer.proto | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index b6849c10d7d..7736a14b9ae 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -63,7 +63,8 @@ message LoadBalanceRequest { } message InitialLoadBalanceRequest { - // Name of load balanced service (IE, service.grpc.gslb.google.com) + // Name of load balanced service (IE, service.grpc.gslb.google.com). Its + // length should be less than 128 bytes. string name = 1; } @@ -95,7 +96,8 @@ message InitialLoadBalanceResponse { // This is an application layer redirect that indicates the client should use // the specified server for load balancing. When this field is non-empty in // the response, the client should open a separate connection to the - // load_balancer_delegate and call the BalanceLoad method. + // load_balancer_delegate and call the BalanceLoad method. Its length should + // be less than 64 bytes. string load_balancer_delegate = 1; // This interval defines how often the client should send the client stats From 98da61ba109405f173fbe8e11d169ab41fc407fb Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sat, 29 Oct 2016 08:46:31 +0200 Subject: [PATCH 08/28] gRPC LB fixes from end two end testing --- src/core/ext/lb_policy/grpclb/grpclb.c | 632 +++++++++--------- .../ext/lb_policy/round_robin/round_robin.c | 39 +- .../client/secure/secure_channel_create.c | 2 +- .../security/transport/security_connector.c | 4 +- 4 files changed, 364 insertions(+), 313 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 6da4febf266..d2af27e7bfc 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -43,30 +43,26 @@ * policy to select from this list of LB server backends. * * The first time the policy gets a request for a pick, a ping, or to exit the - * idle state, \a query_for_backends() is called. It creates an instance of \a - * lb_client_data, an internal struct meant to contain the data associated with - * the internal communication with the LB server. This instance is created via - * \a lb_client_data_create(). There, the call over lb_channel to pick-first - * from {a1..an} is created, the \a LoadBalancingRequest message is assembled - * and all necessary callbacks for the progress of the internal call configured. + * idle state, \a query_for_backends_locked() is called. This function sets up + * and initiates the internal communication with the LB server. In particular, + * it's responsible for instantiating the internal *streaming* call to the LB + * server (whichever address from {a1..an} pick-first chose). This call is + * serviced by two callbacks, \a srv_status_rcvd and \a res_rcvd. The former + * will be called when the call to the LB server completes. This can happen if + * the LB server closes the connection or if this policy itself cancels the call + * (for example because it's shutting down). If the call fails with + * UNIMPLEMENTED, the original picks/pings will fail. This signals that there's + * a misconfiguration somewhere: at least one of {a1..an} isn't an LB server, + * which contradicts the LB bit being set. If the internal call times out, the + * usual behavior of pick-first applies, continuing to pick from the list + * {a1..an}. * - * Back in \a query_for_backends(), the internal *streaming* call to the LB - * server (whichever address from {a1..an} pick-first chose) is kicked off. - * It'll progress over the callbacks configured in \a lb_client_data_create() - * (see the field docstrings of \a lb_client_data for more details). - * - * If the call fails with UNIMPLEMENTED, the original call will also fail. - * There's a misconfiguration somewhere: at least one of {a1..an} isn't a LB - * server, which contradicts the LB bit being set. If the internal call times - * out, the usual behavior of pick-first applies, continuing to pick from the - * list {a1..an}. - * - * Upon sucesss, a \a LoadBalancingResponse is expected in \a res_recv_cb. An - * invalid one results in the termination of the streaming call. A new streaming - * call should be created if possible, failing the original call otherwise. - * For a valid \a LoadBalancingResponse, the server list of actual backends is - * extracted. A Round Robin policy will be created from this list. There are two - * possible scenarios: + * Upon sucesss, the incoming \a LoadBalancingResponse is processed by \a + * res_recv. An invalid one results in the termination of the streaming call. A + * new streaming call should be created if possible, failing the original call + * otherwise. For a valid \a LoadBalancingResponse, the server list of actual + * backends is extracted. A Round Robin policy will be created from this list. + * There are two possible scenarios: * * 1. This is the first server list received. There was no previous instance of * the Round Robin policy. \a rr_handover_locked() will instantiate the RR @@ -120,12 +116,20 @@ #include "src/core/ext/lb_policy/grpclb/grpclb.h" #include "src/core/ext/lb_policy/grpclb/load_balancer_api.h" #include "src/core/lib/channel/channel_args.h" +#include "src/core/lib/iomgr/sockaddr.h" #include "src/core/lib/iomgr/sockaddr_utils.h" +#include "src/core/lib/iomgr/timer.h" +#include "src/core/lib/support/backoff.h" #include "src/core/lib/support/string.h" #include "src/core/lib/surface/call.h" #include "src/core/lib/surface/channel.h" #include "src/core/lib/transport/static_metadata.h" +#define BACKOFF_MULTIPLIER 1.6 +#define BACKOFF_JITTER 0.2 +#define BACKOFF_MIN_SECONDS 10 +#define BACKOFF_MAX_SECONDS 60 + int grpc_lb_glb_trace = 0; /* add lb_token of selected subchannel (address) to the call's initial @@ -174,13 +178,12 @@ typedef struct wrapped_rr_closure_arg { static void wrapped_rr_closure(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { wrapped_rr_closure_arg *wc_arg = arg; - if (wc_arg->rr_policy != NULL) { - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Unreffing RR (0x%" PRIxPTR ")", - (intptr_t)wc_arg->rr_policy); - } - GRPC_LB_POLICY_UNREF(exec_ctx, wc_arg->rr_policy, "wrapped_rr_closure"); + GPR_ASSERT(wc_arg->wrapped_closure != NULL); + grpc_exec_ctx_sched(exec_ctx, wc_arg->wrapped_closure, GRPC_ERROR_REF(error), + NULL); + + if (wc_arg->rr_policy != NULL) { /* if target is NULL, no pick has been made by the RR policy (eg, all * addresses failed to connect). There won't be any user_data/token * available */ @@ -189,10 +192,12 @@ static void wrapped_rr_closure(grpc_exec_ctx *exec_ctx, void *arg, wc_arg->lb_token_mdelem_storage, GRPC_MDELEM_REF(wc_arg->lb_token)); } + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "Unreffing RR (0x%" PRIxPTR ")", + (intptr_t)wc_arg->rr_policy); + } + GRPC_LB_POLICY_UNREF(exec_ctx, wc_arg->rr_policy, "wrapped_rr_closure"); } - GPR_ASSERT(wc_arg->wrapped_closure != NULL); - grpc_exec_ctx_sched(exec_ctx, wc_arg->wrapped_closure, GRPC_ERROR_REF(error), - NULL); GPR_ASSERT(wc_arg->free_when_done != NULL); gpr_free(wc_arg->free_when_done); } @@ -264,7 +269,6 @@ static void add_pending_ping(pending_ping **root, grpc_closure *notify) { * glb_lb_policy */ typedef struct rr_connectivity_data rr_connectivity_data; -struct lb_client_data; static const grpc_lb_policy_vtable glb_lb_policy_vtable; typedef struct glb_lb_policy { /** base policy: must be first */ @@ -296,20 +300,53 @@ typedef struct glb_lb_policy { * response has arrived. */ grpc_grpclb_serverlist *serverlist; - /** addresses from \a serverlist */ - grpc_lb_addresses *addresses; - /** list of picks that are waiting on RR's policy connectivity */ pending_pick *pending_picks; /** list of pings that are waiting on RR's policy connectivity */ pending_ping *pending_pings; - /** client data associated with the LB server communication */ - struct lb_client_data *lb_client; + bool shutting_down; + + /************************************************************/ + /* client data associated with the LB server communication */ + /************************************************************/ + /* called once initial metadata's been sent */ + grpc_closure md_sent; + + /* called once the LoadBalanceRequest has been sent to the LB server. See + * src/proto/grpc/.../load_balancer.proto */ + grpc_closure req_sent; + + /* A response from the LB server has been received (or error). Process it */ + grpc_closure res_rcvd; + + /* ... and the status from the LB server has been received */ + grpc_closure srv_status_rcvd; + + grpc_call *lb_call; /* streaming call to the LB server, */ + + grpc_metadata_array initial_metadata_recv; /* initial MD from LB server */ + grpc_metadata_array trailing_metadata_recv; /* trailing MD from LB server */ + + /* what's being sent to the LB server. Note that its value may vary if the LB + * server indicates a redirect. */ + grpc_byte_buffer *request_payload; + + /* response from the LB server, if any. Processed in res_recv_cb() */ + grpc_byte_buffer *response_payload; + + /* the call's status and status detailset in srv_status_rcvd_cb() */ + grpc_status_code lb_call_status; + char *lb_call_status_details; + size_t lb_call_status_details_capacity; + + /** LB call retry backoff state */ + gpr_backoff lb_call_backoff_state; + + /** LB call retry timer */ + grpc_timer lb_call_retry_timer; - /** for tracking of the RR connectivity */ - rr_connectivity_data *rr_connectivity; } glb_lb_policy; /* Keeps track and reacts to changes in connectivity of the RR instance */ @@ -427,7 +464,6 @@ static grpc_lb_addresses *process_serverlist( ++addr_idx; } GPR_ASSERT(addr_idx == num_valid); - return lb_addresses; } @@ -448,7 +484,7 @@ static bool pick_from_internal_rr_locked( gpr_log(GPR_INFO, "Unreffing RR (0x%" PRIxPTR ")", (intptr_t)wc_arg->rr_policy); } - GRPC_LB_POLICY_UNREF(exec_ctx, wc_arg->rr_policy, "glb_pick"); + GRPC_LB_POLICY_UNREF(exec_ctx, wc_arg->rr_policy, "glb_pick_sync"); /* add the load reporting initial metadata */ initial_metadata_add_lb_token(pick_args->initial_metadata, @@ -461,7 +497,6 @@ static bool pick_from_internal_rr_locked( * pending pick list inside the RR policy (glb_policy->rr_policy). * Eventually, wrapped_on_complete will be called, which will -among other * things- add the LB token to the call's initial metadata */ - return pick_done; } @@ -470,54 +505,69 @@ static grpc_lb_policy *create_rr_locked( glb_lb_policy *glb_policy) { GPR_ASSERT(serverlist != NULL && serverlist->num_servers > 0); - if (glb_policy->addresses != NULL) { - /* dispose of the previous version */ - grpc_lb_addresses_destroy(glb_policy->addresses); - } - glb_policy->addresses = process_serverlist(serverlist); - grpc_lb_policy_args args; memset(&args, 0, sizeof(args)); args.client_channel_factory = glb_policy->cc_factory; + grpc_lb_addresses *addresses = process_serverlist(serverlist); // Replace the LB addresses in the channel args that we pass down to // the subchannel. static const char *keys_to_remove[] = {GRPC_ARG_LB_ADDRESSES}; - const grpc_arg arg = - grpc_lb_addresses_create_channel_arg(glb_policy->addresses); + const grpc_arg arg = grpc_lb_addresses_create_channel_arg(addresses); args.args = grpc_channel_args_copy_and_add_and_remove( glb_policy->args, keys_to_remove, GPR_ARRAY_SIZE(keys_to_remove), &arg, 1); grpc_lb_policy *rr = grpc_lb_policy_create(exec_ctx, "round_robin", &args); + GPR_ASSERT(rr != NULL); + grpc_lb_addresses_destroy(addresses); grpc_channel_args_destroy(args.args); - return rr; } +static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error); +/* glb_policy->rr_policy may be NULL (initial handover) */ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy, grpc_error *error) { GPR_ASSERT(glb_policy->serverlist != NULL && glb_policy->serverlist->num_servers > 0); + + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "RR handover. Old RR: %p", (void *)glb_policy->rr_policy); + } + if (glb_policy->rr_policy != NULL) { + /* if we are phasing out an existing RR instance, unref it. */ + GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "rr_handover_locked"); + } + glb_policy->rr_policy = create_rr_locked(exec_ctx, glb_policy->serverlist, glb_policy); - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Created RR policy (0x%" PRIxPTR ")", - (intptr_t)glb_policy->rr_policy); + gpr_log(GPR_INFO, "Created RR policy (%p)", (void *)glb_policy->rr_policy); } + GPR_ASSERT(glb_policy->rr_policy != NULL); grpc_pollset_set_add_pollset_set(exec_ctx, glb_policy->rr_policy->interested_parties, glb_policy->base.interested_parties); - glb_policy->rr_connectivity->state = grpc_lb_policy_check_connectivity( + + rr_connectivity_data *rr_connectivity = + gpr_malloc(sizeof(rr_connectivity_data)); + memset(rr_connectivity, 0, sizeof(rr_connectivity_data)); + grpc_closure_init(&rr_connectivity->on_change, glb_rr_connectivity_changed, + rr_connectivity); + rr_connectivity->glb_policy = glb_policy; + rr_connectivity->state = grpc_lb_policy_check_connectivity( exec_ctx, glb_policy->rr_policy, &error); - grpc_lb_policy_notify_on_state_change( - exec_ctx, glb_policy->rr_policy, &glb_policy->rr_connectivity->state, - &glb_policy->rr_connectivity->on_change); + grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, - glb_policy->rr_connectivity->state, - GRPC_ERROR_REF(error), "rr_handover"); + rr_connectivity->state, GRPC_ERROR_REF(error), + "rr_handover"); + /* subscribe */ + grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, + &rr_connectivity->state, + &rr_connectivity->on_change); grpc_lb_policy_exit_idle(exec_ctx, glb_policy->rr_policy); /* flush pending ops */ @@ -551,35 +601,24 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { + /* If shutdown or error free the arg. Rely on the rest of the code to set the + * right grpclb status. */ rr_connectivity_data *rr_conn_data = arg; glb_lb_policy *glb_policy = rr_conn_data->glb_policy; - if (rr_conn_data->state == GRPC_CHANNEL_SHUTDOWN) { - if (glb_policy->serverlist != NULL) { - /* a RR policy is shutting down but there's a serverlist available -> - * perform a handover */ - gpr_mu_lock(&glb_policy->mu); - rr_handover_locked(exec_ctx, glb_policy, error); - gpr_mu_unlock(&glb_policy->mu); - } else { - /* shutting down and no new serverlist available. Bail out. */ - gpr_free(rr_conn_data); - } + if (rr_conn_data->state != GRPC_CHANNEL_SHUTDOWN) { + gpr_mu_lock(&glb_policy->mu); + /* RR not shutting down. Mimic the RR's policy state */ + grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, + rr_conn_data->state, GRPC_ERROR_REF(error), + "glb_rr_connectivity_changed"); + /* resubscribe */ + grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, + &rr_conn_data->state, + &rr_conn_data->on_change); + gpr_mu_unlock(&glb_policy->mu); } else { - if (error == GRPC_ERROR_NONE) { - gpr_mu_lock(&glb_policy->mu); - /* RR not shutting down. Mimic the RR's policy state */ - grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, - rr_conn_data->state, GRPC_ERROR_REF(error), - "glb_rr_connectivity_changed"); - /* resubscribe */ - grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, - &rr_conn_data->state, - &rr_conn_data->on_change); - gpr_mu_unlock(&glb_policy->mu); - } else { /* error */ - gpr_free(rr_conn_data); - } + gpr_free(rr_conn_data); } } @@ -682,18 +721,11 @@ static grpc_lb_policy *glb_create(grpc_exec_ctx *exec_ctx, return NULL; } - rr_connectivity_data *rr_connectivity = - gpr_malloc(sizeof(rr_connectivity_data)); - memset(rr_connectivity, 0, sizeof(rr_connectivity_data)); - grpc_closure_init(&rr_connectivity->on_change, glb_rr_connectivity_changed, - rr_connectivity); - rr_connectivity->glb_policy = glb_policy; - glb_policy->rr_connectivity = rr_connectivity; - grpc_lb_policy_init(&glb_policy->base, &glb_lb_policy_vtable); gpr_mu_init(&glb_policy->mu); grpc_connectivity_state_init(&glb_policy->state_tracker, GRPC_CHANNEL_IDLE, "grpclb"); + return &glb_policy->base; } @@ -710,14 +742,13 @@ static void glb_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { grpc_grpclb_destroy_serverlist(glb_policy->serverlist); } gpr_mu_destroy(&glb_policy->mu); - grpc_lb_addresses_destroy(glb_policy->addresses); gpr_free(glb_policy); } -static void lb_client_data_destroy(struct lb_client_data *lb_client); static void glb_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { glb_lb_policy *glb_policy = (glb_lb_policy *)pol; gpr_mu_lock(&glb_policy->mu); + glb_policy->shutting_down = true; pending_pick *pp = glb_policy->pending_picks; glb_policy->pending_picks = NULL; @@ -741,15 +772,15 @@ static void glb_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { } if (glb_policy->rr_policy) { - /* unsubscribe */ - grpc_lb_policy_notify_on_state_change( - exec_ctx, glb_policy->rr_policy, NULL, - &glb_policy->rr_connectivity->on_change); GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "glb_shutdown"); } - lb_client_data_destroy(glb_policy->lb_client); - glb_policy->lb_client = NULL; + if (glb_policy->started_picking) { + if (glb_policy->lb_call != NULL) { + grpc_call_cancel(glb_policy->lb_call, NULL); + /* srv_status_rcvd_cb will pick up the cancellation and clean up */ + } + } grpc_connectivity_state_set( exec_ctx, &glb_policy->state_tracker, GRPC_CHANNEL_SHUTDOWN, @@ -780,17 +811,12 @@ static void glb_cancel_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, GRPC_ERROR_UNREF(error); } -static grpc_call *lb_client_data_get_call(struct lb_client_data *lb_client); static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, uint32_t initial_metadata_flags_mask, uint32_t initial_metadata_flags_eq, grpc_error *error) { glb_lb_policy *glb_policy = (glb_lb_policy *)pol; gpr_mu_lock(&glb_policy->mu); - if (glb_policy->lb_client != NULL) { - /* cancel the call to the load balancer service, if any */ - grpc_call_cancel(lb_client_data_get_call(glb_policy->lb_client), NULL); - } pending_pick *pp = glb_policy->pending_picks; glb_policy->pending_picks = NULL; while (pp != NULL) { @@ -810,18 +836,20 @@ static void glb_cancel_picks(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, GRPC_ERROR_UNREF(error); } -static void query_for_backends(grpc_exec_ctx *exec_ctx, - glb_lb_policy *glb_policy); -static void start_picking(grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy) { +static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, + glb_lb_policy *glb_policy); +static void start_picking_locked(grpc_exec_ctx *exec_ctx, + glb_lb_policy *glb_policy) { glb_policy->started_picking = true; - query_for_backends(exec_ctx, glb_policy); + gpr_backoff_reset(&glb_policy->lb_call_backoff_state); + query_for_backends_locked(exec_ctx, glb_policy); } static void glb_exit_idle(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { glb_lb_policy *glb_policy = (glb_lb_policy *)pol; gpr_mu_lock(&glb_policy->mu); if (!glb_policy->started_picking) { - start_picking(exec_ctx, glb_policy); + start_picking_locked(exec_ctx, glb_policy); } gpr_mu_unlock(&glb_policy->mu); } @@ -847,8 +875,8 @@ static int glb_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, if (glb_policy->rr_policy != NULL) { if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "about to PICK from 0x%" PRIxPTR "", - (intptr_t)glb_policy->rr_policy); + gpr_log(GPR_INFO, "grpclb %p about to PICK from RR %p", + (void *)glb_policy, (void *)glb_policy->rr_policy); } GRPC_LB_POLICY_REF(glb_policy->rr_policy, "glb_pick"); @@ -865,11 +893,17 @@ static int glb_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pick_done = pick_from_internal_rr_locked(exec_ctx, glb_policy->rr_policy, pick_args, target, wc_arg); } else { + if (grpc_lb_glb_trace) { + gpr_log(GPR_DEBUG, + "No RR policy in grpclb instance %p. Adding to grpclb's pending " + "picks", + (void *)(glb_policy)); + } add_pending_pick(&glb_policy->pending_picks, pick_args, target, on_complete); if (!glb_policy->started_picking) { - start_picking(exec_ctx, glb_policy); + start_picking_locked(exec_ctx, glb_policy); } pick_done = false; } @@ -898,7 +932,7 @@ static void glb_ping_one(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, } else { add_pending_ping(&glb_policy->pending_pings, closure); if (!glb_policy->started_picking) { - start_picking(exec_ctx, glb_policy); + start_picking_locked(exec_ctx, glb_policy); } } gpr_mu_unlock(&glb_policy->mu); @@ -916,250 +950,181 @@ static void glb_notify_on_state_change(grpc_exec_ctx *exec_ctx, gpr_mu_unlock(&glb_policy->mu); } -/* - * lb_client_data - * - * Used internally for the client call to the LB */ -typedef struct lb_client_data { - gpr_mu mu; - - /* called once initial metadata's been sent */ - grpc_closure md_sent; - - /* called once the LoadBalanceRequest has been sent to the LB server. See - * src/proto/grpc/.../load_balancer.proto */ - grpc_closure req_sent; - - /* A response from the LB server has been received (or error). Process it */ - grpc_closure res_rcvd; - - /* After the client has sent a close to the LB server */ - grpc_closure close_sent; - - /* ... and the status from the LB server has been received */ - grpc_closure srv_status_rcvd; - - grpc_call *lb_call; /* streaming call to the LB server, */ - gpr_timespec deadline; /* for the streaming call to the LB server */ - - grpc_metadata_array initial_metadata_recv; /* initial MD from LB server */ - grpc_metadata_array trailing_metadata_recv; /* trailing MD from LB server */ - - /* what's being sent to the LB server. Note that its value may vary if the LB - * server indicates a redirect. */ - grpc_byte_buffer *request_payload; - - /* response from the LB server, if any. Processed in res_recv_cb() */ - grpc_byte_buffer *response_payload; - - /* the call's status and status detailset in srv_status_rcvd_cb() */ - grpc_status_code status; - char *status_details; - size_t status_details_capacity; - - /* pointer back to the enclosing policy */ - glb_lb_policy *glb_policy; -} lb_client_data; - -static void md_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); -static void req_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); -static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); -static void close_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error); static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); - -static lb_client_data *lb_client_data_create(glb_lb_policy *glb_policy) { +static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); +static void lb_client_init(glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->server_name != NULL); GPR_ASSERT(glb_policy->server_name[0] != '\0'); - lb_client_data *lb_client = gpr_malloc(sizeof(lb_client_data)); - memset(lb_client, 0, sizeof(lb_client_data)); - - gpr_mu_init(&lb_client->mu); - grpc_closure_init(&lb_client->md_sent, md_sent_cb, lb_client); - - grpc_closure_init(&lb_client->req_sent, req_sent_cb, lb_client); - grpc_closure_init(&lb_client->res_rcvd, res_recv_cb, lb_client); - grpc_closure_init(&lb_client->close_sent, close_sent_cb, lb_client); - grpc_closure_init(&lb_client->srv_status_rcvd, srv_status_rcvd_cb, lb_client); - - lb_client->deadline = glb_policy->deadline; - /* Note the following LB call progresses every time there's activity in \a * glb_policy->base.interested_parties, which is comprised of the polling * entities from \a client_channel. */ - lb_client->lb_call = grpc_channel_create_pollset_set_call( + glb_policy->lb_call = grpc_channel_create_pollset_set_call( glb_policy->lb_channel, NULL, GRPC_PROPAGATE_DEFAULTS, glb_policy->base.interested_parties, "/grpc.lb.v1.LoadBalancer/BalanceLoad", glb_policy->server_name, - lb_client->deadline, NULL); + glb_policy->deadline, NULL); - grpc_metadata_array_init(&lb_client->initial_metadata_recv); - grpc_metadata_array_init(&lb_client->trailing_metadata_recv); + grpc_metadata_array_init(&glb_policy->initial_metadata_recv); + grpc_metadata_array_init(&glb_policy->trailing_metadata_recv); grpc_grpclb_request *request = grpc_grpclb_request_create(glb_policy->server_name); gpr_slice request_payload_slice = grpc_grpclb_request_encode(request); - lb_client->request_payload = + glb_policy->request_payload = grpc_raw_byte_buffer_create(&request_payload_slice, 1); gpr_slice_unref(request_payload_slice); grpc_grpclb_request_destroy(request); - lb_client->status_details = NULL; - lb_client->status_details_capacity = 0; - lb_client->glb_policy = glb_policy; - return lb_client; + glb_policy->lb_call_status_details = NULL; + glb_policy->lb_call_status_details_capacity = 0; + + grpc_closure_init(&glb_policy->srv_status_rcvd, srv_status_rcvd_cb, + glb_policy); + grpc_closure_init(&glb_policy->res_rcvd, res_recv_cb, glb_policy); + + gpr_backoff_init(&glb_policy->lb_call_backoff_state, BACKOFF_MULTIPLIER, + BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, + BACKOFF_MAX_SECONDS * 1000); } -static void lb_client_data_destroy(lb_client_data *lb_client) { - grpc_call_destroy(lb_client->lb_call); - grpc_metadata_array_destroy(&lb_client->initial_metadata_recv); - grpc_metadata_array_destroy(&lb_client->trailing_metadata_recv); +static void lb_client_destroy(glb_lb_policy *glb_policy) { + GPR_ASSERT(glb_policy->lb_call != NULL); + grpc_call_destroy(glb_policy->lb_call); + glb_policy->lb_call = NULL; - grpc_byte_buffer_destroy(lb_client->request_payload); + grpc_metadata_array_destroy(&glb_policy->initial_metadata_recv); + grpc_metadata_array_destroy(&glb_policy->trailing_metadata_recv); - gpr_free(lb_client->status_details); - gpr_mu_destroy(&lb_client->mu); - gpr_free(lb_client); -} -static grpc_call *lb_client_data_get_call(lb_client_data *lb_client) { - return lb_client->lb_call; + grpc_byte_buffer_destroy(glb_policy->request_payload); + gpr_free(glb_policy->lb_call_status_details); } /* * Auxiliary functions and LB client callbacks. */ -static void query_for_backends(grpc_exec_ctx *exec_ctx, - glb_lb_policy *glb_policy) { +static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, + glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->lb_channel != NULL); + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "query_for_backends_locked"); + lb_client_init(glb_policy); + + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "Query for backends (grpclb: %p, lb_call: %p)", + (void *)glb_policy, (void *)glb_policy->lb_call); + } + GPR_ASSERT(glb_policy->lb_call != NULL); - glb_policy->lb_client = lb_client_data_create(glb_policy); grpc_call_error call_error; - grpc_op ops[1]; + grpc_op ops[4]; memset(ops, 0, sizeof(ops)); + grpc_op *op = ops; op->op = GRPC_OP_SEND_INITIAL_METADATA; op->data.send_initial_metadata.count = 0; op->flags = 0; op->reserved = NULL; op++; - call_error = grpc_call_start_batch_and_execute( - exec_ctx, glb_policy->lb_client->lb_call, ops, (size_t)(op - ops), - &glb_policy->lb_client->md_sent); - GPR_ASSERT(GRPC_CALL_OK == call_error); - op = ops; - op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; - op->data.recv_status_on_client.trailing_metadata = - &glb_policy->lb_client->trailing_metadata_recv; - op->data.recv_status_on_client.status = &glb_policy->lb_client->status; - op->data.recv_status_on_client.status_details = - &glb_policy->lb_client->status_details; - op->data.recv_status_on_client.status_details_capacity = - &glb_policy->lb_client->status_details_capacity; + op->op = GRPC_OP_RECV_INITIAL_METADATA; + op->data.recv_initial_metadata = &glb_policy->initial_metadata_recv; op->flags = 0; op->reserved = NULL; op++; - call_error = grpc_call_start_batch_and_execute( - exec_ctx, glb_policy->lb_client->lb_call, ops, (size_t)(op - ops), - &glb_policy->lb_client->srv_status_rcvd); - GPR_ASSERT(GRPC_CALL_OK == call_error); -} - -static void md_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - lb_client_data *lb_client = arg; - GPR_ASSERT(lb_client->lb_call); - grpc_op ops[1]; - memset(ops, 0, sizeof(ops)); - grpc_op *op = ops; + GPR_ASSERT(glb_policy->request_payload != NULL); op->op = GRPC_OP_SEND_MESSAGE; - op->data.send_message = lb_client->request_payload; + op->data.send_message = glb_policy->request_payload; op->flags = 0; op->reserved = NULL; op++; - grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, lb_client->lb_call, ops, (size_t)(op - ops), - &lb_client->req_sent); - GPR_ASSERT(GRPC_CALL_OK == call_error); -} -static void req_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - lb_client_data *lb_client = arg; - GPR_ASSERT(lb_client->lb_call); - - grpc_op ops[2]; - memset(ops, 0, sizeof(ops)); - grpc_op *op = ops; - - op->op = GRPC_OP_RECV_INITIAL_METADATA; - op->data.recv_initial_metadata = &lb_client->initial_metadata_recv; + op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; + op->data.recv_status_on_client.trailing_metadata = + &glb_policy->trailing_metadata_recv; + op->data.recv_status_on_client.status = &glb_policy->lb_call_status; + op->data.recv_status_on_client.status_details = + &glb_policy->lb_call_status_details; + op->data.recv_status_on_client.status_details_capacity = + &glb_policy->lb_call_status_details_capacity; op->flags = 0; op->reserved = NULL; op++; + call_error = grpc_call_start_batch_and_execute(exec_ctx, glb_policy->lb_call, + ops, (size_t)(op - ops), + &glb_policy->srv_status_rcvd); + GPR_ASSERT(GRPC_CALL_OK == call_error); + op = ops; op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = &lb_client->response_payload; + op->data.recv_message = &glb_policy->response_payload; op->flags = 0; op->reserved = NULL; op++; - grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, lb_client->lb_call, ops, (size_t)(op - ops), - &lb_client->res_rcvd); + call_error = grpc_call_start_batch_and_execute(exec_ctx, glb_policy->lb_call, + ops, (size_t)(op - ops), + &glb_policy->res_rcvd); GPR_ASSERT(GRPC_CALL_OK == call_error); } static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - lb_client_data *lb_client = arg; + glb_lb_policy *glb_policy = arg; + grpc_op ops[2]; memset(ops, 0, sizeof(ops)); grpc_op *op = ops; - if (lb_client->response_payload != NULL) { + if (glb_policy->response_payload != NULL) { + gpr_backoff_reset(&glb_policy->lb_call_backoff_state); /* Received data from the LB server. Look inside - * lb_client->response_payload, for a serverlist. */ + * glb_policy->response_payload, for a serverlist. */ grpc_byte_buffer_reader bbr; - grpc_byte_buffer_reader_init(&bbr, lb_client->response_payload); + grpc_byte_buffer_reader_init(&bbr, glb_policy->response_payload); gpr_slice response_slice = grpc_byte_buffer_reader_readall(&bbr); - grpc_byte_buffer_destroy(lb_client->response_payload); + grpc_byte_buffer_destroy(glb_policy->response_payload); grpc_grpclb_serverlist *serverlist = grpc_grpclb_response_parse_serverlist(response_slice); if (serverlist != NULL) { + GPR_ASSERT(glb_policy->lb_call != NULL); gpr_slice_unref(response_slice); if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Serverlist with %lu servers received", (unsigned long)serverlist->num_servers); + /* TODO(dgq): this needs to work with ipv6. */ + for (size_t i = 0; i < serverlist->num_servers; ++i) { + grpc_resolved_address addr; + struct sockaddr_in *sa = (struct sockaddr_in *)&addr.addr; + addr.len = sizeof(struct sockaddr_in); + sa->sin_family = AF_INET; + sa->sin_port = htons((uint16_t)serverlist->servers[i]->port); + memcpy(&sa->sin_addr, serverlist->servers[i]->ip_address.bytes, + serverlist->servers[i]->ip_address.size); + char *ipport; + grpc_sockaddr_to_string(&ipport, &addr, false); + gpr_log(GPR_INFO, "Serverlist[%lu]: %s", (unsigned long)i, ipport); + gpr_free(ipport); + } } /* update serverlist */ if (serverlist->num_servers > 0) { - gpr_mu_lock(&lb_client->glb_policy->mu); - if (grpc_grpclb_serverlist_equals(lb_client->glb_policy->serverlist, - serverlist)) { + gpr_mu_lock(&glb_policy->mu); + if (grpc_grpclb_serverlist_equals(glb_policy->serverlist, serverlist)) { if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Incoming server list identical to current, ignoring."); } } else { /* new serverlist */ - if (lb_client->glb_policy->serverlist != NULL) { + if (glb_policy->serverlist != NULL) { /* dispose of the old serverlist */ - grpc_grpclb_destroy_serverlist(lb_client->glb_policy->serverlist); + grpc_grpclb_destroy_serverlist(glb_policy->serverlist); } /* and update the copy in the glb_lb_policy instance */ - lb_client->glb_policy->serverlist = serverlist; - } - if (lb_client->glb_policy->rr_policy == NULL) { - /* initial "handover", in this case from a null RR policy, meaning - * it'll just create the first RR policy instance */ - rr_handover_locked(exec_ctx, lb_client->glb_policy, error); - } else { - /* unref the RR policy, eventually leading to its substitution with a - * new one constructed from the received serverlist (see - * glb_rr_connectivity_changed) */ - GRPC_LB_POLICY_UNREF(exec_ctx, lb_client->glb_policy->rr_policy, - "serverlist_received"); + glb_policy->serverlist = serverlist; + + rr_handover_locked(exec_ctx, glb_policy, error); } - gpr_mu_unlock(&lb_client->glb_policy->mu); + gpr_mu_unlock(&glb_policy->mu); } else { if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, @@ -1170,13 +1135,13 @@ static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { /* keep listening for serverlist updates */ op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = &lb_client->response_payload; + op->data.recv_message = &glb_policy->response_payload; op->flags = 0; op->reserved = NULL; op++; const grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, lb_client->lb_call, ops, (size_t)(op - ops), - &lb_client->res_rcvd); /* loop */ + exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), + &glb_policy->res_rcvd); /* loop */ GPR_ASSERT(GRPC_CALL_OK == call_error); return; } @@ -1185,42 +1150,105 @@ static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { gpr_log(GPR_ERROR, "Invalid LB response received: '%s'", gpr_dump_slice(response_slice, GPR_DUMP_ASCII)); gpr_slice_unref(response_slice); - - /* Disconnect from server returning invalid response. */ - op->op = GRPC_OP_SEND_CLOSE_FROM_CLIENT; - op->flags = 0; - op->reserved = NULL; - op++; - grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, lb_client->lb_call, ops, (size_t)(op - ops), - &lb_client->close_sent); - GPR_ASSERT(GRPC_CALL_OK == call_error); + grpc_call_cancel(glb_policy->lb_call, NULL); + /* srv_status_rcvd_cb will pick up the cancellation and clean up */ } - /* empty payload: call cancelled by server. Cleanups happening in - * srv_status_rcvd_cb */ + /* else, empty payload: call cancelled by server. */ + grpc_metadata_array_destroy(&glb_policy->initial_metadata_recv); } -static void close_sent_cb(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error) { - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, - "Close from LB client sent. Waiting from server status now"); +static void lb_call_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { + glb_lb_policy *glb_policy = arg; + gpr_mu_lock(&glb_policy->mu); + + if (!glb_policy->shutting_down) { + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "Restaring call to LB server (grpclb %p)", + (void *)glb_policy); + } + GPR_ASSERT(glb_policy->lb_call == NULL); + query_for_backends_locked(exec_ctx, glb_policy); } + gpr_mu_unlock(&glb_policy->mu); + + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, + "grpclb_on_retry_timer"); } static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { - lb_client_data *lb_client = arg; + glb_lb_policy *glb_policy = arg; + gpr_mu_lock(&glb_policy->mu); + + GPR_ASSERT(glb_policy->lb_call != NULL); + if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, - "status from lb server received. Status = %d, Details = '%s', " - "Capacity " - "= %lu", - lb_client->status, lb_client->status_details, - (unsigned long)lb_client->status_details_capacity); + gpr_log(GPR_DEBUG, + "Status from LB server received. Status = %d, Details = '%s', " + "(call: %p)", + glb_policy->lb_call_status, glb_policy->lb_call_status_details, + (void *)glb_policy->lb_call); + } + + if (glb_policy->lb_call_status == GRPC_STATUS_UNIMPLEMENTED) { + char *failing_server = grpc_call_get_peer(glb_policy->lb_call); + char *error_desc; + gpr_asprintf(&error_desc, "Invalid LB server '%s'", failing_server); + gpr_free(failing_server); + /* flush pending ops */ + pending_pick *pp; + while ((pp = glb_policy->pending_picks)) { + glb_policy->pending_picks = pp->next; + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "Cancelling pending pick: %s", error_desc); + } + grpc_exec_ctx_sched(exec_ctx, + &pp->wrapped_on_complete_arg.wrapper_closure, + GRPC_ERROR_CREATE(error_desc), NULL); + } + + pending_ping *pping; + while ((pping = glb_policy->pending_pings)) { + glb_policy->pending_pings = pping->next; + if (grpc_lb_glb_trace) { + gpr_log(GPR_INFO, "Cancelling pending ping: %s", error_desc); + } + grpc_exec_ctx_sched(exec_ctx, &pping->wrapped_notify_arg.wrapper_closure, + GRPC_ERROR_CREATE(error_desc), NULL); + } + gpr_free(error_desc); + } + + const bool was_cancelled = + (glb_policy->lb_call_status == GRPC_STATUS_CANCELLED); + + /* We need to performe cleanups no matter what. */ + lb_client_destroy(glb_policy); + + if (!glb_policy->shutting_down) { + GPR_ASSERT(!was_cancelled); + /* if we aren't shutting down, restart the LB client call after some time */ + gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); + gpr_timespec next_try = + gpr_backoff_step(&glb_policy->lb_call_backoff_state, now); + if (grpc_lb_glb_trace) { + gpr_log(GPR_DEBUG, "Connection to LB server lost (grpclb: %p)...", + (void *)glb_policy); + gpr_timespec timeout = gpr_time_sub(next_try, now); + if (gpr_time_cmp(timeout, gpr_time_0(timeout.clock_type)) > 0) { + gpr_log(GPR_DEBUG, "... retrying in %" PRId64 ".%09d seconds.", + timeout.tv_sec, timeout.tv_nsec); + } else { + gpr_log(GPR_DEBUG, "... retrying immediately."); + } + } + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "grpclb_retry_timer"); + grpc_timer_init(exec_ctx, &glb_policy->lb_call_retry_timer, next_try, + lb_call_on_retry_timer, glb_policy, now); } - /* TODO(dgq): deal with stream termination properly (fire up another one? - * fail the original call?) */ + gpr_mu_unlock(&glb_policy->mu); + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, "srv_status_rcvd_cb"); } /* Code wiring the policy with the rest of the core */ diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 37a9b18b970..5f530d54fe8 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -120,6 +120,8 @@ typedef struct { grpc_connectivity_state connectivity_state; /** the subchannel's target user data */ void *user_data; + /** vtable to operate over \a user_data */ + grpc_lb_user_data_vtable user_data_vtable; } subchannel_data; struct round_robin_lb_policy { @@ -186,9 +188,13 @@ static void advance_last_picked_locked(round_robin_lb_policy *p) { } if (grpc_lb_round_robin_trace) { - gpr_log(GPR_DEBUG, "[READYLIST] ADVANCED LAST PICK. NOW AT NODE %p (SC %p)", - (void *)p->ready_list_last_pick, - (void *)p->ready_list_last_pick->subchannel); + gpr_log(GPR_DEBUG, + "[READYLIST, RR: %p] ADVANCED LAST PICK. NOW AT NODE %p (SC %p, " + "CSC %p)", + (void *)p, (void *)p->ready_list_last_pick, + (void *)p->ready_list_last_pick->subchannel, + (void *)grpc_subchannel_get_connected_subchannel( + p->ready_list_last_pick->subchannel)); } } @@ -255,9 +261,15 @@ static void remove_disconnected_sc_locked(round_robin_lb_policy *p, static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { round_robin_lb_policy *p = (round_robin_lb_policy *)pol; ready_list *elem; + + if (grpc_lb_round_robin_trace) { + gpr_log(GPR_DEBUG, "Destroying Round Robin policy at %p", (void*)pol); + } + for (size_t i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; - GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin"); + GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin_destroy"); + sd->user_data_vtable.destroy(sd->user_data); gpr_free(sd); } @@ -285,6 +297,9 @@ static void rr_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { size_t i; gpr_mu_lock(&p->mu); + if (grpc_lb_round_robin_trace) { + gpr_log(GPR_DEBUG, "Shutting down Round Robin policy at %p", pol); + } p->shutdown = 1; while ((pp = p->pending_picks)) { @@ -296,7 +311,7 @@ static void rr_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { } grpc_connectivity_state_set( exec_ctx, &p->state_tracker, GRPC_CHANNEL_SHUTDOWN, - GRPC_ERROR_CREATE("Channel Shutdown"), "shutdown"); + GRPC_ERROR_CREATE("Channel Shutdown"), "rr_shutdown"); for (i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; grpc_subchannel_notify_on_state_change(exec_ctx, sd->subchannel, NULL, NULL, @@ -395,6 +410,11 @@ static int rr_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, pending_pick *pp; ready_list *selected; gpr_mu_lock(&p->mu); + + if (grpc_lb_round_robin_trace) { + gpr_log(GPR_INFO, "Round Robin %p trying to pick", pol); + } + if ((selected = peek_next_connected_locked(p))) { /* readily available, report right away */ *target = GRPC_CONNECTED_SUBCHANNEL_REF( @@ -435,7 +455,6 @@ static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, subchannel_data *sd = arg; round_robin_lb_policy *p = sd->policy; pending_pick *pp; - ready_list *selected; int unref = 0; @@ -456,12 +475,14 @@ static void rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, /* at this point we know there's at least one suitable subchannel. Go * ahead and pick one and notify the pending suitors in * p->pending_picks. This preemtively replicates rr_pick()'s actions. */ - selected = peek_next_connected_locked(p); + ready_list *selected = peek_next_connected_locked(p); + GPR_ASSERT(selected != NULL); if (p->pending_picks != NULL) { /* if the selected subchannel is going to be used for the pending * picks, update the last picked pointer */ advance_last_picked_locked(p); } + while ((pp = p->pending_picks)) { p->pending_picks = pp->next; @@ -653,7 +674,9 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, sd->policy = p; sd->index = subchannel_idx; sd->subchannel = subchannel; - sd->user_data = addresses->addresses[i].user_data; + sd->user_data_vtable = *addresses->user_data_vtable; + sd->user_data = + sd->user_data_vtable.copy(addresses->addresses[i].user_data); ++subchannel_idx; grpc_closure_init(&sd->connectivity_changed_closure, rr_connectivity_changed, sd); diff --git a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c index 57e1a8ec01f..d0ac72a0114 100644 --- a/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c +++ b/src/core/ext/transport/chttp2/client/secure/secure_channel_create.c @@ -347,7 +347,7 @@ grpc_channel *grpc_secure_channel_create(grpc_channel_credentials *creds, &exec_ctx, &f->base, target, GRPC_CLIENT_CHANNEL_TYPE_REGULAR, new_args); // Clean up. GRPC_SECURITY_CONNECTOR_UNREF(&f->security_connector->base, - "client_channel_factory_create_channel"); + "secure_client_channel_factory_create_channel"); grpc_channel_args_destroy(new_args); grpc_client_channel_factory_unref(&exec_ctx, &f->base); grpc_exec_ctx_finish(&exec_ctx); diff --git a/src/core/lib/security/transport/security_connector.c b/src/core/lib/security/transport/security_connector.c index 0eca46eb525..ebf72a3abb4 100644 --- a/src/core/lib/security/transport/security_connector.c +++ b/src/core/lib/security/transport/security_connector.c @@ -210,11 +210,11 @@ void grpc_security_connector_unref(grpc_security_connector *sc) { } static void connector_pointer_arg_destroy(void *p) { - GRPC_SECURITY_CONNECTOR_UNREF(p, "connector_pointer_arg"); + GRPC_SECURITY_CONNECTOR_UNREF(p, "connector_pointer_arg_destroy"); } static void *connector_pointer_arg_copy(void *p) { - return GRPC_SECURITY_CONNECTOR_REF(p, "connector_pointer_arg"); + return GRPC_SECURITY_CONNECTOR_REF(p, "connector_pointer_arg_copy"); } static int connector_pointer_cmp(void *a, void *b) { return GPR_ICMP(a, b); } From f37cd4f84448d9a9bb96cb16e1f616811e42f9af Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Sun, 30 Oct 2016 15:18:27 +0100 Subject: [PATCH 09/28] Updated max length of load balanced service to 255 bytes --- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c | 9 +-------- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 6 +++--- src/proto/grpc/lb/v1/load_balancer.options | 2 +- src/proto/grpc/lb/v1/load_balancer.proto | 2 +- 4 files changed, 6 insertions(+), 13 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c index afecb716fb0..c90d3f9ff59 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c @@ -106,14 +106,7 @@ PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) #endif #if !defined(PB_FIELD_16BIT) && !defined(PB_FIELD_32BIT) -/* If you get an error here, it means that you need to define PB_FIELD_16BIT - * compile-time option. You can do that in pb.h or on compiler command line. - * - * The reason you need to do this is that some of your messages contain tag - * numbers or field sizes that are larger than what can fit in the default - * 8 bit descriptors. - */ -PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceRequest, client_stats) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, initial_response) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, server_list) < 256 && pb_membersize(grpc_lb_v1_InitialLoadBalanceResponse, client_stats_report_interval) < 256 && pb_membersize(grpc_lb_v1_ServerList, servers) < 256 && pb_membersize(grpc_lb_v1_ServerList, expiration_interval) < 256), YOU_MUST_DEFINE_PB_FIELD_16BIT_FOR_MESSAGES_grpc_lb_v1_Duration_grpc_lb_v1_LoadBalanceRequest_grpc_lb_v1_InitialLoadBalanceRequest_grpc_lb_v1_ClientStats_grpc_lb_v1_LoadBalanceResponse_grpc_lb_v1_InitialLoadBalanceResponse_grpc_lb_v1_ServerList_grpc_lb_v1_Server) +#error Field descriptor for grpc_lb_v1_InitialLoadBalanceRequest.name is too large. Define PB_FIELD_16BIT to fix this. #endif diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index e36d0966f85..81e89254d17 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -66,7 +66,7 @@ typedef struct _grpc_lb_v1_Duration { typedef struct _grpc_lb_v1_InitialLoadBalanceRequest { bool has_name; - char name[128]; + char name[256]; /* @@protoc_insertion_point(struct:grpc_lb_v1_InitialLoadBalanceRequest) */ } grpc_lb_v1_InitialLoadBalanceRequest; @@ -166,8 +166,8 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; /* Maximum encoded size of messages (where known) */ #define grpc_lb_v1_Duration_size 22 -#define grpc_lb_v1_LoadBalanceRequest_size 169 -#define grpc_lb_v1_InitialLoadBalanceRequest_size 131 +#define grpc_lb_v1_LoadBalanceRequest_size 297 +#define grpc_lb_v1_InitialLoadBalanceRequest_size 259 #define grpc_lb_v1_ClientStats_size 33 #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index 7fbd44b9ded..46e27fa2c11 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,4 +1,4 @@ -grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 +grpc.lb.v1.InitialLoadBalanceRequest.name max_size:256 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 diff --git a/src/proto/grpc/lb/v1/load_balancer.proto b/src/proto/grpc/lb/v1/load_balancer.proto index 7736a14b9ae..44a5150a7e0 100644 --- a/src/proto/grpc/lb/v1/load_balancer.proto +++ b/src/proto/grpc/lb/v1/load_balancer.proto @@ -64,7 +64,7 @@ message LoadBalanceRequest { message InitialLoadBalanceRequest { // Name of load balanced service (IE, service.grpc.gslb.google.com). Its - // length should be less than 128 bytes. + // length should be less than 256 bytes. string name = 1; } From fd635add1c96344398fafb33fb2ba86a22055a5d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 31 Oct 2016 14:20:56 -0700 Subject: [PATCH 10/28] clang-format --- test/cpp/end2end/round_robin_end2end_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/cpp/end2end/round_robin_end2end_test.cc b/test/cpp/end2end/round_robin_end2end_test.cc index fe0cd0baa40..6ee7417db87 100644 --- a/test/cpp/end2end/round_robin_end2end_test.cc +++ b/test/cpp/end2end/round_robin_end2end_test.cc @@ -107,8 +107,8 @@ class RoundRobinEnd2endTest : public ::testing::Test { uri << "127.0.0.1:" << servers_[i]->port_ << ","; } uri << "127.0.0.1:" << servers_[servers_.size() - 1]->port_; - std::shared_ptr channel = CreateCustomChannel( - uri.str(), InsecureChannelCredentials(), args); + std::shared_ptr channel = + CreateCustomChannel(uri.str(), InsecureChannelCredentials(), args); stub_ = grpc::testing::EchoTestService::NewStub(channel); } From 69259d425111696dd2df5a243e8b714db3dc3dac Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Mon, 31 Oct 2016 14:34:10 -0700 Subject: [PATCH 11/28] Add resource quota support to uv TCP code --- src/core/lib/iomgr/endpoint_pair_uv.c | 5 +- src/core/lib/iomgr/resource_quota.c | 7 ++ src/core/lib/iomgr/resource_quota.h | 5 + src/core/lib/iomgr/tcp_client_uv.c | 36 +++++-- src/core/lib/iomgr/tcp_server_uv.c | 24 ++++- src/core/lib/iomgr/tcp_uv.c | 133 +++++++++++++++++--------- src/core/lib/iomgr/tcp_uv.h | 4 +- 7 files changed, 154 insertions(+), 60 deletions(-) diff --git a/src/core/lib/iomgr/endpoint_pair_uv.c b/src/core/lib/iomgr/endpoint_pair_uv.c index 7941e203886..ff24894c6db 100644 --- a/src/core/lib/iomgr/endpoint_pair_uv.c +++ b/src/core/lib/iomgr/endpoint_pair_uv.c @@ -41,8 +41,9 @@ #include "src/core/lib/iomgr/endpoint_pair.h" -grpc_endpoint_pair grpc_iomgr_create_endpoint_pair(const char *name, - size_t read_slice_size) { +grpc_endpoint_pair grpc_iomgr_create_endpoint_pair( + const char *name, grpc_resource_quota *resource_quota, + size_t read_slice_size) { grpc_endpoint_pair endpoint_pair; // TODO(mlumish): implement this properly under libuv GPR_ASSERT(false && diff --git a/src/core/lib/iomgr/resource_quota.c b/src/core/lib/iomgr/resource_quota.c index bfc905845d6..40c847a1b36 100644 --- a/src/core/lib/iomgr/resource_quota.c +++ b/src/core/lib/iomgr/resource_quota.c @@ -712,3 +712,10 @@ void grpc_resource_user_alloc_slices( grpc_resource_user_alloc(exec_ctx, slice_allocator->resource_user, count * length, &slice_allocator->on_allocated); } + +gpr_slice grpc_resource_user_slice_malloc(grpc_exec_ctx *exec_ctx, + grpc_resource_user *resource_user, + size_t size) { + grpc_resource_user_alloc(exec_ctx, resource_user, size, NULL); + return ru_slice_create(resource_user, size); +} diff --git a/src/core/lib/iomgr/resource_quota.h b/src/core/lib/iomgr/resource_quota.h index 6dfac55f88c..da68f21a2c7 100644 --- a/src/core/lib/iomgr/resource_quota.h +++ b/src/core/lib/iomgr/resource_quota.h @@ -221,4 +221,9 @@ void grpc_resource_user_alloc_slices( grpc_resource_user_slice_allocator *slice_allocator, size_t length, size_t count, gpr_slice_buffer *dest); +/* Allocate one slice of length \a size synchronously. */ +gpr_slice grpc_resource_user_slice_malloc(grpc_exec_ctx *exec_ctx, + grpc_resource_user *resource_user, + size_t size); + #endif /* GRPC_CORE_LIB_IOMGR_RESOURCE_QUOTA_H */ diff --git a/src/core/lib/iomgr/tcp_client_uv.c b/src/core/lib/iomgr/tcp_client_uv.c index 62746670420..b07f9ceffa3 100644 --- a/src/core/lib/iomgr/tcp_client_uv.c +++ b/src/core/lib/iomgr/tcp_client_uv.c @@ -54,9 +54,12 @@ typedef struct grpc_uv_tcp_connect { grpc_endpoint **endpoint; int refs; char *addr_name; + grpc_resource_quota *resource_quota; } grpc_uv_tcp_connect; -static void uv_tcp_connect_cleanup(grpc_uv_tcp_connect *connect) { +static void uv_tcp_connect_cleanup(grpc_exec_ctx *exec_ctx, + grpc_uv_tcp_connect *connect) { + grpc_resource_quota_internal_unref(exec_ctx, connect->resource_quota); gpr_free(connect); } @@ -74,7 +77,7 @@ static void uv_tc_on_alarm(grpc_exec_ctx *exec_ctx, void *acp, } done = (--connect->refs == 0); if (done) { - uv_tcp_connect_cleanup(connect); + uv_tcp_connect_cleanup(exec_ctx, connect); } } @@ -86,8 +89,8 @@ static void uv_tc_on_connect(uv_connect_t *req, int status) { grpc_closure *closure = connect->closure; grpc_timer_cancel(&exec_ctx, &connect->alarm); if (status == 0) { - *connect->endpoint = - grpc_tcp_create(connect->tcp_handle, connect->addr_name); + *connect->endpoint = grpc_tcp_create( + connect->tcp_handle, connect->resource_quota, connect->addr_name); } else { error = GRPC_ERROR_CREATE("Failed to connect to remote host"); error = grpc_error_set_int(error, GRPC_ERROR_INT_ERRNO, -status); @@ -105,7 +108,7 @@ static void uv_tc_on_connect(uv_connect_t *req, int status) { } done = (--connect->refs == 0); if (done) { - uv_tcp_connect_cleanup(connect); + uv_tcp_connect_cleanup(&exec_ctx, connect); } grpc_exec_ctx_sched(&exec_ctx, closure, error, NULL); grpc_exec_ctx_finish(&exec_ctx); @@ -114,16 +117,31 @@ static void uv_tc_on_connect(uv_connect_t *req, int status) { static void tcp_client_connect_impl(grpc_exec_ctx *exec_ctx, grpc_closure *closure, grpc_endpoint **ep, grpc_pollset_set *interested_parties, + const grpc_channel_args *channel_args, const grpc_resolved_address *resolved_addr, gpr_timespec deadline) { grpc_uv_tcp_connect *connect; + grpc_resource_quota *resource_quota = grpc_resource_quota_create(NULL); + (void)channel_args; (void)interested_parties; + + if (channel_args != NULL) { + for (size_t i = 0; i < channel_args->num_args; i++) { + if (0 == strcmp(channel_args->args[i].key, GRPC_ARG_RESOURCE_QUOTA)) { + grpc_resource_quota_internal_unref(exec_ctx, resource_quota); + resource_quota = grpc_resource_quota_internal_ref( + channel_args->args[i].value.pointer.p); + } + } + } + connect = gpr_malloc(sizeof(grpc_uv_tcp_connect)); memset(connect, 0, sizeof(grpc_uv_tcp_connect)); connect->closure = closure; connect->endpoint = ep; connect->tcp_handle = gpr_malloc(sizeof(uv_tcp_t)); connect->addr_name = grpc_sockaddr_to_uri(resolved_addr); + connect->resource_quota = resource_quota; uv_tcp_init(uv_default_loop(), connect->tcp_handle); connect->connect_req.data = connect; // TODO(murgatroid99): figure out what the return value here means @@ -138,16 +156,18 @@ static void tcp_client_connect_impl(grpc_exec_ctx *exec_ctx, // overridden by api_fuzzer.c void (*grpc_tcp_client_connect_impl)( grpc_exec_ctx *exec_ctx, grpc_closure *closure, grpc_endpoint **ep, - grpc_pollset_set *interested_parties, const grpc_resolved_address *addr, + grpc_pollset_set *interested_parties, const grpc_channel_args *channel_args, + const grpc_resolved_address *addr, gpr_timespec deadline) = tcp_client_connect_impl; void grpc_tcp_client_connect(grpc_exec_ctx *exec_ctx, grpc_closure *closure, grpc_endpoint **ep, grpc_pollset_set *interested_parties, + const grpc_channel_args *channel_args, const grpc_resolved_address *addr, gpr_timespec deadline) { - grpc_tcp_client_connect_impl(exec_ctx, closure, ep, interested_parties, addr, - deadline); + grpc_tcp_client_connect_impl(exec_ctx, closure, ep, interested_parties, + channel_args, addr, deadline); } #endif /* GRPC_UV */ diff --git a/src/core/lib/iomgr/tcp_server_uv.c b/src/core/lib/iomgr/tcp_server_uv.c index 73e4db3d650..b5b9b92a20a 100644 --- a/src/core/lib/iomgr/tcp_server_uv.c +++ b/src/core/lib/iomgr/tcp_server_uv.c @@ -76,13 +76,30 @@ struct grpc_tcp_server { /* shutdown callback */ grpc_closure *shutdown_complete; + + grpc_resource_quota *resource_quota; }; -grpc_error *grpc_tcp_server_create(grpc_closure *shutdown_complete, +grpc_error *grpc_tcp_server_create(grpc_exec_ctx *exec_ctx, + grpc_closure *shutdown_complete, const grpc_channel_args *args, grpc_tcp_server **server) { grpc_tcp_server *s = gpr_malloc(sizeof(grpc_tcp_server)); - (void)args; + s->resource_quota = grpc_resource_quota_create(NULL); + for (size_t i = 0; i < (args == NULL ? 0 : args->num_args); i++) { + if (0 == strcmp(GRPC_ARG_RESOURCE_QUOTA, args->args[i].key)) { + if (args->args[i].type == GRPC_ARG_POINTER) { + grpc_resource_quota_internal_unref(exec_ctx, s->resource_quota); + s->resource_quota = + grpc_resource_quota_internal_ref(args->args[i].value.pointer.p); + } else { + grpc_resource_quota_internal_unref(exec_ctx, s->resource_quota); + gpr_free(s); + return GRPC_ERROR_CREATE(GRPC_ARG_RESOURCE_QUOTA + " must be a pointer to a buffer pool"); + } + } + } gpr_ref_init(&s->refs, 1); s->on_accept_cb = NULL; s->on_accept_cb_arg = NULL; @@ -119,6 +136,7 @@ static void finish_shutdown(grpc_exec_ctx *exec_ctx, grpc_tcp_server *s) { gpr_free(sp->handle); gpr_free(sp); } + grpc_resource_quota_internal_unref(exec_ctx, s->resource_quota); gpr_free(s); } @@ -201,7 +219,7 @@ static void on_connect(uv_stream_t *server, int status) { } else { gpr_log(GPR_INFO, "uv_tcp_getpeername error: %s", uv_strerror(status)); } - ep = grpc_tcp_create(client, peer_name_string); + ep = grpc_tcp_create(client, sp->server->resource_quota, peer_name_string); sp->server->on_accept_cb(&exec_ctx, sp->server->on_accept_cb_arg, ep, NULL, &acceptor); grpc_exec_ctx_finish(&exec_ctx); diff --git a/src/core/lib/iomgr/tcp_uv.c b/src/core/lib/iomgr/tcp_uv.c index 3860fe3e9b5..b90e0c80082 100644 --- a/src/core/lib/iomgr/tcp_uv.c +++ b/src/core/lib/iomgr/tcp_uv.c @@ -1,35 +1,35 @@ /* - * - * Copyright 2016, Google Inc. - * All rights reserved. - * - * Redistribution and use in source and binary forms, with or without - * modification, are permitted provided that the following conditions are - * met: - * - * * Redistributions of source code must retain the above copyright - * notice, this list of conditions and the following disclaimer. - * * Redistributions in binary form must reproduce the above - * copyright notice, this list of conditions and the following disclaimer - * in the documentation and/or other materials provided with the - * distribution. - * * Neither the name of Google Inc. nor the names of its - * contributors may be used to endorse or promote products derived from - * this software without specific prior written permission. - * - * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS - * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT - * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR - * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT - * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, - * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT - * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, - * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY - * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT - * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE - * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. - * - */ +* +* Copyright 2016, Google Inc. +* All rights reserved. +* +* Redistribution and use in source and binary forms, with or without +* modification, are permitted provided that the following conditions are +* met: +* +* * Redistributions of source code must retain the above copyright +* notice, this list of conditions and the following disclaimer. +* * Redistributions in binary form must reproduce the above +* copyright notice, this list of conditions and the following disclaimer +* in the documentation and/or other materials provided with the +* distribution. +* * Neither the name of Google Inc. nor the names of its +* contributors may be used to endorse or promote products derived from +* this software without specific prior written permission. +* +* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS +* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT +* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR +* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT +* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, +* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT +* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, +* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY +* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT +* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE +* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +* +*/ #include "src/core/lib/iomgr/port.h" @@ -54,6 +54,9 @@ typedef struct { grpc_endpoint base; gpr_refcount refcount; + uv_write_t write_req; + uv_shutdown_t shutdown_req; + uv_tcp_t *handle; grpc_closure *read_cb; @@ -64,14 +67,23 @@ typedef struct { gpr_slice_buffer *write_slices; uv_buf_t *write_buffers; + grpc_resource_user resource_user; + bool shutting_down; + bool resource_user_shutting_down; + char *peer_string; grpc_pollset *pollset; } grpc_tcp; static void uv_close_callback(uv_handle_t *handle) { gpr_free(handle); } -static void tcp_free(grpc_tcp *tcp) { gpr_free(tcp); } +static void tcp_free(grpc_tcp *tcp) { + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; + grpc_resource_user_destroy(&exec_ctx, &tcp->resource_user); + gpr_free(tcp); + grpc_exec_ctx_finish(&exec_ctx); +} /*#define GRPC_TCP_REFCOUNT_DEBUG*/ #ifdef GRPC_TCP_REFCOUNT_DEBUG @@ -106,11 +118,14 @@ static void tcp_ref(grpc_tcp *tcp) { gpr_ref(&tcp->refcount); } static void alloc_uv_buf(uv_handle_t *handle, size_t suggested_size, uv_buf_t *buf) { + grpc_exec_ctx exec_ctx = GRPC_EXEC_CTX_INIT; grpc_tcp *tcp = handle->data; (void)suggested_size; - tcp->read_slice = gpr_slice_malloc(GRPC_TCP_DEFAULT_READ_SLICE_SIZE); + tcp->read_slice = grpc_resource_user_slice_malloc( + &exec_ctx, &tcp->resource_user, GRPC_TCP_DEFAULT_READ_SLICE_SIZE); buf->base = (char *)GPR_SLICE_START_PTR(tcp->read_slice); buf->len = GPR_SLICE_LENGTH(tcp->read_slice); + grpc_exec_ctx_finish(&exec_ctx); } static void read_callback(uv_stream_t *stream, ssize_t nread, @@ -198,7 +213,8 @@ static void write_callback(uv_write_t *req, int status) { gpr_log(GPR_DEBUG, "write complete on %p: error=%s", tcp, str); } gpr_free(tcp->write_buffers); - gpr_free(req); + grpc_resource_user_free(&exec_ctx, &tcp->resource_user, + sizeof(uv_buf_t) * tcp->write_slices->count); grpc_exec_ctx_sched(&exec_ctx, cb, error, NULL); grpc_exec_ctx_finish(&exec_ctx); } @@ -243,12 +259,15 @@ static void uv_endpoint_write(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep, tcp->write_cb = cb; buffer_count = (unsigned int)tcp->write_slices->count; buffers = gpr_malloc(sizeof(uv_buf_t) * buffer_count); + grpc_resource_user_alloc(exec_ctx, &tcp->resource_user, + sizeof(uv_buf_t) * buffer_count, NULL); for (i = 0; i < buffer_count; i++) { slice = &tcp->write_slices->slices[i]; buffers[i].base = (char *)GPR_SLICE_START_PTR(*slice); buffers[i].len = GPR_SLICE_LENGTH(*slice); } - write_req = gpr_malloc(sizeof(uv_write_t)); + tcp->write_buffers = buffers; + write_req = &tcp->write_req; write_req->data = tcp; TCP_REF(tcp, "write"); // TODO(murgatroid99): figure out what the return value here means @@ -274,13 +293,29 @@ static void uv_add_to_pollset_set(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep, (void)pollset; } -static void shutdown_callback(uv_shutdown_t *req, int status) { gpr_free(req); } +static void shutdown_callback(uv_shutdown_t *req, int status) {} + +static void resource_user_shutdown_done(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { + TCP_UNREF(arg, "resource_user"); +} + +static void uv_resource_user_maybe_shutdown(grpc_exec_ctx *exec_ctx, + grpc_tcp *tcp) { + if (!tcp->resource_user_shutting_down) { + tcp->resource_user_shutting_down = true; + TCP_REF(tcp, "resource_user"); + grpc_resource_user_shutdown( + exec_ctx, &tcp->resource_user, + grpc_closure_create(resource_user_shutdown_done, tcp)); + } +} static void uv_endpoint_shutdown(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep) { grpc_tcp *tcp = (grpc_tcp *)ep; if (!tcp->shutting_down) { tcp->shutting_down = true; - uv_shutdown_t *req = gpr_malloc(sizeof(uv_shutdown_t)); + uv_shutdown_t *req = &tcp->shutdown_req; uv_shutdown(req, (uv_stream_t *)tcp->handle, shutdown_callback); } } @@ -289,6 +324,7 @@ static void uv_destroy(grpc_exec_ctx *exec_ctx, grpc_endpoint *ep) { grpc_network_status_unregister_endpoint(ep); grpc_tcp *tcp = (grpc_tcp *)ep; uv_close((uv_handle_t *)tcp->handle, uv_close_callback); + uv_resource_user_maybe_shutdown(exec_ctx, tcp); TCP_UNREF(tcp, "destroy"); } @@ -297,18 +333,21 @@ static char *uv_get_peer(grpc_endpoint *ep) { return gpr_strdup(tcp->peer_string); } +static grpc_resource_user *uv_get_resource_user(grpc_endpoint *ep) { + grpc_tcp *tcp = (grpc_tcp *)ep; + return &tcp->resource_user; +} + static grpc_workqueue *uv_get_workqueue(grpc_endpoint *ep) { return NULL; } -static grpc_endpoint_vtable vtable = {uv_endpoint_read, - uv_endpoint_write, - uv_get_workqueue, - uv_add_to_pollset, - uv_add_to_pollset_set, - uv_endpoint_shutdown, - uv_destroy, - uv_get_peer}; +static grpc_endpoint_vtable vtable = { + uv_endpoint_read, uv_endpoint_write, uv_get_workqueue, + uv_add_to_pollset, uv_add_to_pollset_set, uv_endpoint_shutdown, + uv_destroy, uv_get_resource_user, uv_get_peer}; -grpc_endpoint *grpc_tcp_create(uv_tcp_t *handle, char *peer_string) { +grpc_endpoint *grpc_tcp_create(uv_tcp_t *handle, + grpc_resource_quota *resource_quota, + char *peer_string) { grpc_tcp *tcp = (grpc_tcp *)gpr_malloc(sizeof(grpc_tcp)); if (grpc_tcp_trace) { @@ -325,6 +364,8 @@ grpc_endpoint *grpc_tcp_create(uv_tcp_t *handle, char *peer_string) { gpr_ref_init(&tcp->refcount, 1); tcp->peer_string = gpr_strdup(peer_string); tcp->shutting_down = false; + tcp->resource_user_shutting_down = false; + grpc_resource_user_init(&tcp->resource_user, resource_quota, peer_string); /* Tell network status tracking code about the new endpoint */ grpc_network_status_register_endpoint(&tcp->base); diff --git a/src/core/lib/iomgr/tcp_uv.h b/src/core/lib/iomgr/tcp_uv.h index eed41151ea7..970fcafe4a5 100644 --- a/src/core/lib/iomgr/tcp_uv.h +++ b/src/core/lib/iomgr/tcp_uv.h @@ -52,6 +52,8 @@ extern int grpc_tcp_trace; #define GRPC_TCP_DEFAULT_READ_SLICE_SIZE 8192 -grpc_endpoint *grpc_tcp_create(uv_tcp_t *handle, char *peer_string); +grpc_endpoint *grpc_tcp_create(uv_tcp_t *handle, + grpc_resource_quota *resource_quota, + char *peer_string); #endif /* GRPC_CORE_LIB_IOMGR_TCP_UV_H */ From 6df607591ce42eee46538726432960e03f6db85d Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 31 Oct 2016 14:39:45 -0700 Subject: [PATCH 12/28] Fix portability problems. --- test/cpp/end2end/round_robin_end2end_test.cc | 28 +++++++++++--------- 1 file changed, 15 insertions(+), 13 deletions(-) diff --git a/test/cpp/end2end/round_robin_end2end_test.cc b/test/cpp/end2end/round_robin_end2end_test.cc index 6ee7417db87..f4ca3baa548 100644 --- a/test/cpp/end2end/round_robin_end2end_test.cc +++ b/test/cpp/end2end/round_robin_end2end_test.cc @@ -63,6 +63,8 @@ namespace { // every call to the Echo RPC. class MyTestServiceImpl : public TestServiceImpl { public: + MyTestServiceImpl() : request_count_(0) {} + Status Echo(ServerContext* context, const EchoRequest* request, EchoResponse* response) GRPC_OVERRIDE { { @@ -79,7 +81,7 @@ class MyTestServiceImpl : public TestServiceImpl { private: mutex mu_; - int request_count_ = 0; + int request_count_; }; class RoundRobinEnd2endTest : public ::testing::Test { @@ -93,8 +95,8 @@ class RoundRobinEnd2endTest : public ::testing::Test { } void TearDown() GRPC_OVERRIDE { - for (const auto& server : servers_) { - server->Shutdown(); + for (size_t i = 0; i < servers_.size(); ++i) { + servers_[i]->Shutdown(); } } @@ -135,17 +137,15 @@ class RoundRobinEnd2endTest : public ::testing::Test { gpr_log(GPR_INFO, "starting server on port %d", port_); std::mutex mu; std::condition_variable cond; - thread_ = new std::thread([this, server_host, &mu, &cond]() { - Start(server_host); - lock_guard lock(mu); - cond.notify_one(); - }); + thread_ = new std::thread( + std::bind(&ServerData::Start, this, server_host, &mu, &cond)); unique_lock lock(mu); cond.wait(lock); gpr_log(GPR_INFO, "server startup complete"); } - void Start(const grpc::string& server_host) { + void Start(const grpc::string& server_host, std::mutex* mu, + std::condition_variable* cond) { std::ostringstream server_address; server_address << server_host << ":" << port_; ServerBuilder builder; @@ -153,6 +153,8 @@ class RoundRobinEnd2endTest : public ::testing::Test { InsecureServerCredentials()); builder.RegisterService(&service_); server_ = builder.BuildAndStart(); + lock_guard lock(*mu); + cond->notify_one(); } void Shutdown() { @@ -175,8 +177,8 @@ TEST_F(RoundRobinEnd2endTest, PickFirst) { SendRpc(kNumServers); // All requests should have gone to a single server. bool found = false; - for (const auto& server : servers_) { - const int request_count = server->service_.request_count(); + for (size_t i = 0; i < servers_.size(); ++i) { + const int request_count = servers_[i]->service_.request_count(); if (request_count == kNumServers) { found = true; } else { @@ -193,8 +195,8 @@ TEST_F(RoundRobinEnd2endTest, RoundRobin) { ResetStub(true /* round_robin */); SendRpc(kNumServers); // One request should have gone to each server. - for (const auto& server : servers_) { - EXPECT_EQ(1, server->service_.request_count()); + for (size_t i = 0; i < servers_.size(); ++i) { + EXPECT_EQ(1, servers_[i]->service_.request_count()); } } From 3f265857f41c88ef7c7d7a8f1ace89a1a3f839ba Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 31 Oct 2016 14:42:22 -0700 Subject: [PATCH 13/28] Fix memory leak. --- test/cpp/end2end/round_robin_end2end_test.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/cpp/end2end/round_robin_end2end_test.cc b/test/cpp/end2end/round_robin_end2end_test.cc index f4ca3baa548..796b01cd4ac 100644 --- a/test/cpp/end2end/round_robin_end2end_test.cc +++ b/test/cpp/end2end/round_robin_end2end_test.cc @@ -130,15 +130,15 @@ class RoundRobinEnd2endTest : public ::testing::Test { int port_; std::unique_ptr server_; MyTestServiceImpl service_; - std::thread* thread_; + std::unique_ptr thread_; explicit ServerData(const grpc::string& server_host) { port_ = grpc_pick_unused_port_or_die(); gpr_log(GPR_INFO, "starting server on port %d", port_); std::mutex mu; std::condition_variable cond; - thread_ = new std::thread( - std::bind(&ServerData::Start, this, server_host, &mu, &cond)); + thread_.reset(new std::thread( + std::bind(&ServerData::Start, this, server_host, &mu, &cond))); unique_lock lock(mu); cond.wait(lock); gpr_log(GPR_INFO, "server startup complete"); From 5a4005772d4d5025c6ffc704dcfec7fcc6cafe97 Mon Sep 17 00:00:00 2001 From: Masood Malekghassemi Date: Mon, 31 Oct 2016 17:20:15 -0700 Subject: [PATCH 14/28] Don't use the stack so much Because internally we like to keep our thread stacks tiny. --- .../end2end/tests/resource_quota_server.c | 39 +++++++++++++------ 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/test/core/end2end/tests/resource_quota_server.c b/test/core/end2end/tests/resource_quota_server.c index fbd4986dfba..47c5ed6abb7 100644 --- a/test/core/end2end/tests/resource_quota_server.c +++ b/test/core/end2end/tests/resource_quota_server.c @@ -137,17 +137,22 @@ void resource_quota_server(grpc_end2end_test_config config) { * will be verified on completion. */ gpr_slice request_payload_slice = generate_random_slice(); - grpc_call *client_calls[NUM_CALLS]; - grpc_call *server_calls[NUM_CALLS]; - grpc_metadata_array initial_metadata_recv[NUM_CALLS]; - grpc_metadata_array trailing_metadata_recv[NUM_CALLS]; - grpc_metadata_array request_metadata_recv[NUM_CALLS]; - grpc_call_details call_details[NUM_CALLS]; - grpc_status_code status[NUM_CALLS]; - char *details[NUM_CALLS]; - size_t details_capacity[NUM_CALLS]; - grpc_byte_buffer *request_payload_recv[NUM_CALLS]; - int was_cancelled[NUM_CALLS]; + grpc_call **client_calls = malloc(sizeof(grpc_call *) * NUM_CALLS); + grpc_call **server_calls = malloc(sizeof(grpc_call *) * NUM_CALLS); + grpc_metadata_array *initial_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_metadata_array *trailing_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_metadata_array *request_metadata_recv = + malloc(sizeof(grpc_metadata_array) * NUM_CALLS); + grpc_call_details *call_details = + malloc(sizeof(grpc_call_details) * NUM_CALLS); + grpc_status_code *status = malloc(sizeof(grpc_status_code) * NUM_CALLS); + char **details = malloc(sizeof(char *) * NUM_CALLS); + size_t *details_capacity = malloc(sizeof(size_t) * NUM_CALLS); + grpc_byte_buffer **request_payload_recv = + malloc(sizeof(grpc_byte_buffer *) * NUM_CALLS); + int *was_cancelled = malloc(sizeof(int) * NUM_CALLS); grpc_call_error error; int pending_client_calls = 0; int pending_server_start_calls = 0; @@ -356,6 +361,18 @@ void resource_quota_server(grpc_end2end_test_config config) { gpr_slice_unref(request_payload_slice); grpc_resource_quota_unref(resource_quota); + free(client_calls); + free(server_calls); + free(initial_metadata_recv); + free(trailing_metadata_recv); + free(request_metadata_recv); + free(call_details); + free(status); + free(details); + free(details_capacity); + free(request_payload_recv); + free(was_cancelled); + end_test(&f); config.tear_down_data(&f); } From 7ec291330c4442bc2d4adc1f93ff8f7bd9149c14 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 04:09:05 +0100 Subject: [PATCH 15/28] PR comments --- src/core/ext/lb_policy/grpclb/grpclb.c | 253 ++++++++---------- .../ext/lb_policy/round_robin/round_robin.c | 14 +- test/cpp/grpclb/grpclb_test.cc | 16 +- 3 files changed, 133 insertions(+), 150 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index d2af27e7bfc..9a608fd4776 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -47,15 +47,12 @@ * and initiates the internal communication with the LB server. In particular, * it's responsible for instantiating the internal *streaming* call to the LB * server (whichever address from {a1..an} pick-first chose). This call is - * serviced by two callbacks, \a srv_status_rcvd and \a res_rcvd. The former - * will be called when the call to the LB server completes. This can happen if - * the LB server closes the connection or if this policy itself cancels the call - * (for example because it's shutting down). If the call fails with - * UNIMPLEMENTED, the original picks/pings will fail. This signals that there's - * a misconfiguration somewhere: at least one of {a1..an} isn't an LB server, - * which contradicts the LB bit being set. If the internal call times out, the - * usual behavior of pick-first applies, continuing to pick from the list - * {a1..an}. + * serviced by two callbacks, \a lb_on_server_status_received and \a + * lb_on_response_received. The former will be called when the call to the LB + * server completes. This can happen if the LB server closes the connection or + * if this policy itself cancels the call (for example because it's shutting + * down).If the internal call times out, the usual behavior of pick-first + * applies, continuing to pick from the list {a1..an}. * * Upon sucesss, the incoming \a LoadBalancingResponse is processed by \a * res_recv. An invalid one results in the termination of the streaming call. A @@ -80,10 +77,10 @@ * Once a RR policy instance is in place (and getting updated as described), * calls to for a pick, a ping or a cancellation will be serviced right away by * forwarding them to the RR instance. Any time there's no RR policy available - * (ie, right after the creation of the gRPCLB policy, if an empty serverlist - * is received, etc), pick/ping requests are added to a list of pending - * picks/pings to be flushed and serviced as part of \a rr_handover_locked() the - * moment the RR policy instance becomes available. + * (ie, right after the creation of the gRPCLB policy, if an empty serverlist is + * received, etc), pick/ping requests are added to a list of pending picks/pings + * to be flushed and serviced as part of \a rr_handover_locked() the moment the + * RR policy instance becomes available. * * \see https://github.com/grpc/grpc/blob/master/doc/load-balancing.md for the * high level design and details. */ @@ -311,32 +308,28 @@ typedef struct glb_lb_policy { /************************************************************/ /* client data associated with the LB server communication */ /************************************************************/ - /* called once initial metadata's been sent */ - grpc_closure md_sent; + /* Status from the LB server has been received. This signals the end of the LB + * call. */ + grpc_closure lb_on_server_status_received; - /* called once the LoadBalanceRequest has been sent to the LB server. See - * src/proto/grpc/.../load_balancer.proto */ - grpc_closure req_sent; - - /* A response from the LB server has been received (or error). Process it */ - grpc_closure res_rcvd; - - /* ... and the status from the LB server has been received */ - grpc_closure srv_status_rcvd; + /* A response from the LB server has been received. Process it */ + grpc_closure lb_on_response_received; grpc_call *lb_call; /* streaming call to the LB server, */ - grpc_metadata_array initial_metadata_recv; /* initial MD from LB server */ - grpc_metadata_array trailing_metadata_recv; /* trailing MD from LB server */ + grpc_metadata_array lb_initial_metadata_recv; /* initial MD from LB server */ + grpc_metadata_array + lb_trailing_metadata_recv; /* trailing MD from LB server */ /* what's being sent to the LB server. Note that its value may vary if the LB * server indicates a redirect. */ - grpc_byte_buffer *request_payload; + grpc_byte_buffer *lb_request_payload; - /* response from the LB server, if any. Processed in res_recv_cb() */ - grpc_byte_buffer *response_payload; + /* response from the LB server, if any. Processed in lb_on_response_received() + */ + grpc_byte_buffer *lb_response_payload; - /* the call's status and status detailset in srv_status_rcvd_cb() */ + /* the call's status and status detailset in lb_on_server_status_received() */ grpc_status_code lb_call_status; char *lb_call_status_details; size_t lb_call_status_details_capacity; @@ -346,7 +339,6 @@ typedef struct glb_lb_policy { /** LB call retry timer */ grpc_timer lb_call_retry_timer; - } glb_lb_policy; /* Keeps track and reacts to changes in connectivity of the RR instance */ @@ -395,6 +387,28 @@ static int lb_token_cmp(void *token1, void *token2) { static const grpc_lb_user_data_vtable lb_token_vtable = { lb_token_copy, lb_token_destroy, lb_token_cmp}; +static void parse_server(const grpc_grpclb_server *server, + grpc_resolved_address *addr) { + const uint16_t netorder_port = htons((uint16_t)server->port); + /* the addresses are given in binary format (a in(6)_addr struct) in + * server->ip_address.bytes. */ + const grpc_grpclb_ip_address *ip = &server->ip_address; + memset(addr, 0, sizeof(*addr)); + if (ip->size == 4) { + addr->len = sizeof(struct sockaddr_in); + struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr->addr; + addr4->sin_family = AF_INET; + memcpy(&addr4->sin_addr, ip->bytes, ip->size); + addr4->sin_port = netorder_port; + } else if (ip->size == 16) { + addr->len = sizeof(struct sockaddr_in6); + struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&addr->addr; + addr6->sin6_family = AF_INET; + memcpy(&addr6->sin6_addr, ip->bytes, ip->size); + addr6->sin6_port = netorder_port; + } +} + /* Returns addresses extracted from \a serverlist. */ static grpc_lb_addresses *process_serverlist( const grpc_grpclb_serverlist *serverlist) { @@ -421,25 +435,8 @@ static grpc_lb_addresses *process_serverlist( if (!is_server_valid(serverlist->servers[sl_idx], sl_idx, false)) continue; /* address processing */ - const uint16_t netorder_port = htons((uint16_t)server->port); - /* the addresses are given in binary format (a in(6)_addr struct) in - * server->ip_address.bytes. */ - const grpc_grpclb_ip_address *ip = &server->ip_address; grpc_resolved_address addr; - memset(&addr, 0, sizeof(addr)); - if (ip->size == 4) { - addr.len = sizeof(struct sockaddr_in); - struct sockaddr_in *addr4 = (struct sockaddr_in *)&addr.addr; - addr4->sin_family = AF_INET; - memcpy(&addr4->sin_addr, ip->bytes, ip->size); - addr4->sin_port = netorder_port; - } else if (ip->size == 16) { - addr.len = sizeof(struct sockaddr_in6); - struct sockaddr_in6 *addr6 = (struct sockaddr_in6 *)&addr.addr; - addr6->sin6_family = AF_INET; - memcpy(&addr6->sin6_addr, ip->bytes, ip->size); - addr6->sin6_port = netorder_port; - } + parse_server(server, &addr); /* lb token processing */ void *user_data; @@ -538,7 +535,7 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, } if (glb_policy->rr_policy != NULL) { /* if we are phasing out an existing RR instance, unref it. */ - GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "rr_handover_locked"); + GRPC_LB_POLICY_UNREF(exec_ctx, glb_policy->rr_policy, "rr_handover"); } glb_policy->rr_policy = @@ -565,6 +562,7 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, rr_connectivity->state, GRPC_ERROR_REF(error), "rr_handover"); /* subscribe */ + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "rr_connectiviby_cb"); grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, &rr_connectivity->state, &rr_connectivity->on_change); @@ -606,7 +604,8 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, rr_connectivity_data *rr_conn_data = arg; glb_lb_policy *glb_policy = rr_conn_data->glb_policy; - if (rr_conn_data->state != GRPC_CHANNEL_SHUTDOWN) { + if (rr_conn_data->state != GRPC_CHANNEL_SHUTDOWN && + !glb_policy->shutting_down) { gpr_mu_lock(&glb_policy->mu); /* RR not shutting down. Mimic the RR's policy state */ grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, @@ -618,6 +617,8 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, &rr_conn_data->on_change); gpr_mu_unlock(&glb_policy->mu); } else { + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, + "rr_connectiviby_cb"); gpr_free(rr_conn_data); } } @@ -778,7 +779,8 @@ static void glb_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { if (glb_policy->started_picking) { if (glb_policy->lb_call != NULL) { grpc_call_cancel(glb_policy->lb_call, NULL); - /* srv_status_rcvd_cb will pick up the cancellation and clean up */ + /* lb_on_server_status_received will pick up the cancellation and clean up + */ } } @@ -950,10 +952,11 @@ static void glb_notify_on_state_change(grpc_exec_ctx *exec_ctx, gpr_mu_unlock(&glb_policy->mu); } -static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error); -static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error); -static void lb_client_init(glb_lb_policy *glb_policy) { +static void lb_on_server_status_received(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error); +static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error); +static void lb_call_init(glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->server_name != NULL); GPR_ASSERT(glb_policy->server_name[0] != '\0'); @@ -966,13 +969,13 @@ static void lb_client_init(glb_lb_policy *glb_policy) { "/grpc.lb.v1.LoadBalancer/BalanceLoad", glb_policy->server_name, glb_policy->deadline, NULL); - grpc_metadata_array_init(&glb_policy->initial_metadata_recv); - grpc_metadata_array_init(&glb_policy->trailing_metadata_recv); + grpc_metadata_array_init(&glb_policy->lb_initial_metadata_recv); + grpc_metadata_array_init(&glb_policy->lb_trailing_metadata_recv); grpc_grpclb_request *request = grpc_grpclb_request_create(glb_policy->server_name); gpr_slice request_payload_slice = grpc_grpclb_request_encode(request); - glb_policy->request_payload = + glb_policy->lb_request_payload = grpc_raw_byte_buffer_create(&request_payload_slice, 1); gpr_slice_unref(request_payload_slice); grpc_grpclb_request_destroy(request); @@ -980,24 +983,25 @@ static void lb_client_init(glb_lb_policy *glb_policy) { glb_policy->lb_call_status_details = NULL; glb_policy->lb_call_status_details_capacity = 0; - grpc_closure_init(&glb_policy->srv_status_rcvd, srv_status_rcvd_cb, - glb_policy); - grpc_closure_init(&glb_policy->res_rcvd, res_recv_cb, glb_policy); + grpc_closure_init(&glb_policy->lb_on_server_status_received, + lb_on_server_status_received, glb_policy); + grpc_closure_init(&glb_policy->lb_on_response_received, + lb_on_response_received, glb_policy); gpr_backoff_init(&glb_policy->lb_call_backoff_state, BACKOFF_MULTIPLIER, BACKOFF_JITTER, BACKOFF_MIN_SECONDS * 1000, BACKOFF_MAX_SECONDS * 1000); } -static void lb_client_destroy(glb_lb_policy *glb_policy) { +static void lb_call_destroy(glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->lb_call != NULL); grpc_call_destroy(glb_policy->lb_call); glb_policy->lb_call = NULL; - grpc_metadata_array_destroy(&glb_policy->initial_metadata_recv); - grpc_metadata_array_destroy(&glb_policy->trailing_metadata_recv); + grpc_metadata_array_destroy(&glb_policy->lb_initial_metadata_recv); + grpc_metadata_array_destroy(&glb_policy->lb_trailing_metadata_recv); - grpc_byte_buffer_destroy(glb_policy->request_payload); + grpc_byte_buffer_destroy(glb_policy->lb_request_payload); gpr_free(glb_policy->lb_call_status_details); } @@ -1007,8 +1011,10 @@ static void lb_client_destroy(glb_lb_policy *glb_policy) { static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->lb_channel != NULL); + /* take a weak ref (won't prevent calling of \a glb_shutdown if the strong ref + * count goes to zero) to be unref'd in lb_on_server_status_received */ GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "query_for_backends_locked"); - lb_client_init(glb_policy); + lb_call_init(glb_policy); if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Query for backends (grpclb: %p, lb_call: %p)", @@ -1028,21 +1034,21 @@ static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, op++; op->op = GRPC_OP_RECV_INITIAL_METADATA; - op->data.recv_initial_metadata = &glb_policy->initial_metadata_recv; + op->data.recv_initial_metadata = &glb_policy->lb_initial_metadata_recv; op->flags = 0; op->reserved = NULL; op++; - GPR_ASSERT(glb_policy->request_payload != NULL); + GPR_ASSERT(glb_policy->lb_request_payload != NULL); op->op = GRPC_OP_SEND_MESSAGE; - op->data.send_message = glb_policy->request_payload; + op->data.send_message = glb_policy->lb_request_payload; op->flags = 0; op->reserved = NULL; op++; op->op = GRPC_OP_RECV_STATUS_ON_CLIENT; op->data.recv_status_on_client.trailing_metadata = - &glb_policy->trailing_metadata_recv; + &glb_policy->lb_trailing_metadata_recv; op->data.recv_status_on_client.status = &glb_policy->lb_call_status; op->data.recv_status_on_client.status_details = &glb_policy->lb_call_status_details; @@ -1051,37 +1057,38 @@ static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, op->flags = 0; op->reserved = NULL; op++; - call_error = grpc_call_start_batch_and_execute(exec_ctx, glb_policy->lb_call, - ops, (size_t)(op - ops), - &glb_policy->srv_status_rcvd); + call_error = grpc_call_start_batch_and_execute( + exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), + &glb_policy->lb_on_server_status_received); GPR_ASSERT(GRPC_CALL_OK == call_error); op = ops; op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = &glb_policy->response_payload; + op->data.recv_message = &glb_policy->lb_response_payload; op->flags = 0; op->reserved = NULL; op++; - call_error = grpc_call_start_batch_and_execute(exec_ctx, glb_policy->lb_call, - ops, (size_t)(op - ops), - &glb_policy->res_rcvd); + call_error = grpc_call_start_batch_and_execute( + exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), + &glb_policy->lb_on_response_received); GPR_ASSERT(GRPC_CALL_OK == call_error); } -static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { +static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { glb_lb_policy *glb_policy = arg; grpc_op ops[2]; memset(ops, 0, sizeof(ops)); grpc_op *op = ops; - if (glb_policy->response_payload != NULL) { + if (glb_policy->lb_response_payload != NULL) { gpr_backoff_reset(&glb_policy->lb_call_backoff_state); /* Received data from the LB server. Look inside - * glb_policy->response_payload, for a serverlist. */ + * glb_policy->lb_response_payload, for a serverlist. */ grpc_byte_buffer_reader bbr; - grpc_byte_buffer_reader_init(&bbr, glb_policy->response_payload); + grpc_byte_buffer_reader_init(&bbr, glb_policy->lb_response_payload); gpr_slice response_slice = grpc_byte_buffer_reader_readall(&bbr); - grpc_byte_buffer_destroy(glb_policy->response_payload); + grpc_byte_buffer_destroy(glb_policy->lb_response_payload); grpc_grpclb_serverlist *serverlist = grpc_grpclb_response_parse_serverlist(response_slice); if (serverlist != NULL) { @@ -1090,15 +1097,9 @@ static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { if (grpc_lb_glb_trace) { gpr_log(GPR_INFO, "Serverlist with %lu servers received", (unsigned long)serverlist->num_servers); - /* TODO(dgq): this needs to work with ipv6. */ for (size_t i = 0; i < serverlist->num_servers; ++i) { grpc_resolved_address addr; - struct sockaddr_in *sa = (struct sockaddr_in *)&addr.addr; - addr.len = sizeof(struct sockaddr_in); - sa->sin_family = AF_INET; - sa->sin_port = htons((uint16_t)serverlist->servers[i]->port); - memcpy(&sa->sin_addr, serverlist->servers[i]->ip_address.bytes, - serverlist->servers[i]->ip_address.size); + parse_server(serverlist->servers[i], &addr); char *ipport; grpc_sockaddr_to_string(&ipport, &addr, false); gpr_log(GPR_INFO, "Serverlist[%lu]: %s", (unsigned long)i, ipport); @@ -1132,29 +1133,25 @@ static void res_recv_cb(grpc_exec_ctx *exec_ctx, void *arg, grpc_error *error) { "response with > 0 servers is received"); } } - - /* keep listening for serverlist updates */ - op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = &glb_policy->response_payload; - op->flags = 0; - op->reserved = NULL; - op++; - const grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), - &glb_policy->res_rcvd); /* loop */ - GPR_ASSERT(GRPC_CALL_OK == call_error); - return; + } else { /* serverlist == NULL */ + gpr_log(GPR_ERROR, "Invalid LB response received: '%s'. Ignoring.", + gpr_dump_slice(response_slice, GPR_DUMP_ASCII | GPR_DUMP_HEX)); + gpr_slice_unref(response_slice); } - GPR_ASSERT(serverlist == NULL); - gpr_log(GPR_ERROR, "Invalid LB response received: '%s'", - gpr_dump_slice(response_slice, GPR_DUMP_ASCII)); - gpr_slice_unref(response_slice); - grpc_call_cancel(glb_policy->lb_call, NULL); - /* srv_status_rcvd_cb will pick up the cancellation and clean up */ + /* keep listening for serverlist updates */ + op->op = GRPC_OP_RECV_MESSAGE; + op->data.recv_message = &glb_policy->lb_response_payload; + op->flags = 0; + op->reserved = NULL; + op++; + const grpc_call_error call_error = grpc_call_start_batch_and_execute( + exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), + &glb_policy->lb_on_response_received); /* loop */ + GPR_ASSERT(GRPC_CALL_OK == call_error); + return; } /* else, empty payload: call cancelled by server. */ - grpc_metadata_array_destroy(&glb_policy->initial_metadata_recv); } static void lb_call_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg, @@ -1176,8 +1173,8 @@ static void lb_call_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg, "grpclb_on_retry_timer"); } -static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, - grpc_error *error) { +static void lb_on_server_status_received(grpc_exec_ctx *exec_ctx, void *arg, + grpc_error *error) { glb_lb_policy *glb_policy = arg; gpr_mu_lock(&glb_policy->mu); @@ -1191,40 +1188,11 @@ static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, (void *)glb_policy->lb_call); } - if (glb_policy->lb_call_status == GRPC_STATUS_UNIMPLEMENTED) { - char *failing_server = grpc_call_get_peer(glb_policy->lb_call); - char *error_desc; - gpr_asprintf(&error_desc, "Invalid LB server '%s'", failing_server); - gpr_free(failing_server); - /* flush pending ops */ - pending_pick *pp; - while ((pp = glb_policy->pending_picks)) { - glb_policy->pending_picks = pp->next; - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Cancelling pending pick: %s", error_desc); - } - grpc_exec_ctx_sched(exec_ctx, - &pp->wrapped_on_complete_arg.wrapper_closure, - GRPC_ERROR_CREATE(error_desc), NULL); - } - - pending_ping *pping; - while ((pping = glb_policy->pending_pings)) { - glb_policy->pending_pings = pping->next; - if (grpc_lb_glb_trace) { - gpr_log(GPR_INFO, "Cancelling pending ping: %s", error_desc); - } - grpc_exec_ctx_sched(exec_ctx, &pping->wrapped_notify_arg.wrapper_closure, - GRPC_ERROR_CREATE(error_desc), NULL); - } - gpr_free(error_desc); - } - const bool was_cancelled = (glb_policy->lb_call_status == GRPC_STATUS_CANCELLED); /* We need to performe cleanups no matter what. */ - lb_client_destroy(glb_policy); + lb_call_destroy(glb_policy); if (!glb_policy->shutting_down) { GPR_ASSERT(!was_cancelled); @@ -1248,7 +1216,8 @@ static void srv_status_rcvd_cb(grpc_exec_ctx *exec_ctx, void *arg, lb_call_on_retry_timer, glb_policy, now); } gpr_mu_unlock(&glb_policy->mu); - GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, "srv_status_rcvd_cb"); + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, + "lb_on_server_status_received"); } /* Code wiring the policy with the rest of the core */ diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 5f530d54fe8..36112874502 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -121,7 +121,7 @@ typedef struct { /** the subchannel's target user data */ void *user_data; /** vtable to operate over \a user_data */ - grpc_lb_user_data_vtable user_data_vtable; + const grpc_lb_user_data_vtable *user_data_vtable; } subchannel_data; struct round_robin_lb_policy { @@ -269,7 +269,9 @@ static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { for (size_t i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin_destroy"); - sd->user_data_vtable.destroy(sd->user_data); + if (sd->user_data_vtable != NULL) { + sd->user_data_vtable->destroy(sd->user_data); + } gpr_free(sd); } @@ -298,7 +300,7 @@ static void rr_shutdown(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { gpr_mu_lock(&p->mu); if (grpc_lb_round_robin_trace) { - gpr_log(GPR_DEBUG, "Shutting down Round Robin policy at %p", pol); + gpr_log(GPR_DEBUG, "Shutting down Round Robin policy at %p", (void *)pol); } p->shutdown = 1; @@ -412,7 +414,7 @@ static int rr_pick(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol, gpr_mu_lock(&p->mu); if (grpc_lb_round_robin_trace) { - gpr_log(GPR_INFO, "Round Robin %p trying to pick", pol); + gpr_log(GPR_INFO, "Round Robin %p trying to pick", (void *)pol); } if ((selected = peek_next_connected_locked(p))) { @@ -674,9 +676,9 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, sd->policy = p; sd->index = subchannel_idx; sd->subchannel = subchannel; - sd->user_data_vtable = *addresses->user_data_vtable; + sd->user_data_vtable = addresses->user_data_vtable; sd->user_data = - sd->user_data_vtable.copy(addresses->addresses[i].user_data); + sd->user_data_vtable->copy(addresses->addresses[i].user_data); ++subchannel_idx; grpc_closure_init(&sd->connectivity_changed_closure, rr_connectivity_changed, sd); diff --git a/test/cpp/grpclb/grpclb_test.cc b/test/cpp/grpclb/grpclb_test.cc index b6056f9ae4c..3a92eb56b7f 100644 --- a/test/cpp/grpclb/grpclb_test.cc +++ b/test/cpp/grpclb/grpclb_test.cc @@ -76,10 +76,22 @@ extern "C" { // - Send a serverlist with faulty ip:port addresses (port > 2^16, etc). // - Test reception of invalid serverlist // - Test pinging -// - Test against a non-LB server. That server should return UNIMPLEMENTED and -// the call should fail. +// - Test against a non-LB server. // - Random LB server closing the stream unexpectedly. // - Test using DNS-resolvable names (localhost?) +// +// Findings from end to end testing to be covered here: +// - Handling of LB servers restart, including reconnection after backing-off +// retries. +// - Destruction of load balanced channel (and therefore of grpclb instance) +// while: +// 1) the internal LB call is still active. This should work by virtue +// of the weak reference the LB call holds. The call should be terminated as +// part of the grpclb shutdown process. +// 2) the retry timer is active. Again, the weak reference it holds should +// prevent a premature call to \a glb_destroy. +// - Restart of backend servers with no changes to serverlist. This exercises +// the RR handover mechanism. namespace grpc { namespace { From 82f7f89757cf80e18cf15d316f149bbbb0ea6609 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 1 Nov 2016 09:47:17 -0700 Subject: [PATCH 16/28] Regenerate projects --- tools/run_tests/tests.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 733bfe3b8fd..39daf859b46 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -3003,6 +3003,7 @@ ], "cpu_cost": 1.0, "exclude_configs": [], + "exclude_iomgrs": [], "flaky": false, "gtest": false, "language": "c++", From 87a066e5b1fa42cf057b3fdf4849ca581153bf5b Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 1 Nov 2016 11:06:26 -0700 Subject: [PATCH 17/28] Ran generate_projects.sh --- tools/run_tests/tests.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 4f602fe31e1..878cba35dfa 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -3025,6 +3025,7 @@ ], "cpu_cost": 1.0, "exclude_configs": [], + "exclude_iomgrs": [], "flaky": false, "gtest": false, "language": "c++", From 246c564bb89ec9f52e9ddbdd8a2df4e6df259e08 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 11:16:52 -0700 Subject: [PATCH 18/28] PR comments, take two --- src/core/ext/lb_policy/grpclb/grpclb.c | 37 +++++++++---------- .../ext/lb_policy/round_robin/round_robin.c | 3 +- 2 files changed, 19 insertions(+), 21 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index 9a608fd4776..bfcb9e6418c 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -51,7 +51,7 @@ * lb_on_response_received. The former will be called when the call to the LB * server completes. This can happen if the LB server closes the connection or * if this policy itself cancels the call (for example because it's shutting - * down).If the internal call times out, the usual behavior of pick-first + * down). If the internal call times out, the usual behavior of pick-first * applies, continuing to pick from the list {a1..an}. * * Upon sucesss, the incoming \a LoadBalancingResponse is processed by \a @@ -325,11 +325,10 @@ typedef struct glb_lb_policy { * server indicates a redirect. */ grpc_byte_buffer *lb_request_payload; - /* response from the LB server, if any. Processed in lb_on_response_received() - */ + /* response the LB server, if any. Processed in lb_on_response_received() */ grpc_byte_buffer *lb_response_payload; - /* the call's status and status detailset in lb_on_server_status_received() */ + /* call status code and details, set in lb_on_server_status_received() */ grpc_status_code lb_call_status; char *lb_call_status_details; size_t lb_call_status_details_capacity; @@ -1013,7 +1012,7 @@ static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, GPR_ASSERT(glb_policy->lb_channel != NULL); /* take a weak ref (won't prevent calling of \a glb_shutdown if the strong ref * count goes to zero) to be unref'd in lb_on_server_status_received */ - GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "query_for_backends_locked"); + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "query_for_backends"); lb_call_init(glb_policy); if (grpc_lb_glb_trace) { @@ -1139,19 +1138,21 @@ static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, gpr_slice_unref(response_slice); } - /* keep listening for serverlist updates */ - op->op = GRPC_OP_RECV_MESSAGE; - op->data.recv_message = &glb_policy->lb_response_payload; - op->flags = 0; - op->reserved = NULL; - op++; - const grpc_call_error call_error = grpc_call_start_batch_and_execute( - exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), - &glb_policy->lb_on_response_received); /* loop */ - GPR_ASSERT(GRPC_CALL_OK == call_error); + if (!glb_policy->shutting_down) { + /* keep listening for serverlist updates */ + op->op = GRPC_OP_RECV_MESSAGE; + op->data.recv_message = &glb_policy->lb_response_payload; + op->flags = 0; + op->reserved = NULL; + op++; + const grpc_call_error call_error = grpc_call_start_batch_and_execute( + exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), + &glb_policy->lb_on_response_received); /* loop */ + GPR_ASSERT(GRPC_CALL_OK == call_error); + } return; } - /* else, empty payload: call cancelled by server. */ + /* else, empty payload: call cancelled. */ } static void lb_call_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg, @@ -1188,14 +1189,10 @@ static void lb_on_server_status_received(grpc_exec_ctx *exec_ctx, void *arg, (void *)glb_policy->lb_call); } - const bool was_cancelled = - (glb_policy->lb_call_status == GRPC_STATUS_CANCELLED); - /* We need to performe cleanups no matter what. */ lb_call_destroy(glb_policy); if (!glb_policy->shutting_down) { - GPR_ASSERT(!was_cancelled); /* if we aren't shutting down, restart the LB client call after some time */ gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); gpr_timespec next_try = diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 36112874502..9b20edec92c 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -269,7 +269,8 @@ static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { for (size_t i = 0; i < p->num_subchannels; i++) { subchannel_data *sd = p->subchannels[i]; GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin_destroy"); - if (sd->user_data_vtable != NULL) { + if (sd->user_data != NULL) { + GPR_ASSERT(sd->user_data_vtable); sd->user_data_vtable->destroy(sd->user_data); } gpr_free(sd); From e224a76a6e7523012d8be899240c4cee1ba6f2b4 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 13:00:58 -0700 Subject: [PATCH 19/28] PR comments, take three --- src/core/ext/client_channel/lb_policy.h | 6 +++++ src/core/ext/lb_policy/grpclb/grpclb.c | 25 ++++++++++++------- .../ext/lb_policy/round_robin/round_robin.c | 2 +- 3 files changed, 23 insertions(+), 10 deletions(-) diff --git a/src/core/ext/client_channel/lb_policy.h b/src/core/ext/client_channel/lb_policy.h index 54ad7797920..120c641edc6 100644 --- a/src/core/ext/client_channel/lb_policy.h +++ b/src/core/ext/client_channel/lb_policy.h @@ -109,10 +109,16 @@ struct grpc_lb_policy_vtable { /*#define GRPC_LB_POLICY_REFCOUNT_DEBUG*/ #ifdef GRPC_LB_POLICY_REFCOUNT_DEBUG + +/* Strong references: the policy will shutdown when they reach zero */ #define GRPC_LB_POLICY_REF(p, r) \ grpc_lb_policy_ref((p), __FILE__, __LINE__, (r)) #define GRPC_LB_POLICY_UNREF(exec_ctx, p, r) \ grpc_lb_policy_unref((exec_ctx), (p), __FILE__, __LINE__, (r)) + +/* Weak references: they don't prevent the shutdown of the LB policy. When no + * strong references are left but there are still weak ones, shutdown is called. + * Once the weak reference also reaches zero, the LB policy is destroyed. */ #define GRPC_LB_POLICY_WEAK_REF(p, r) \ grpc_lb_policy_weak_ref((p), __FILE__, __LINE__, (r)) #define GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, p, r) \ diff --git a/src/core/ext/lb_policy/grpclb/grpclb.c b/src/core/ext/lb_policy/grpclb/grpclb.c index bfcb9e6418c..451fbd65de3 100644 --- a/src/core/ext/lb_policy/grpclb/grpclb.c +++ b/src/core/ext/lb_policy/grpclb/grpclb.c @@ -561,7 +561,7 @@ static void rr_handover_locked(grpc_exec_ctx *exec_ctx, rr_connectivity->state, GRPC_ERROR_REF(error), "rr_handover"); /* subscribe */ - GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "rr_connectiviby_cb"); + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "rr_connectivity_cb"); grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, &rr_connectivity->state, &rr_connectivity->on_change); @@ -609,15 +609,15 @@ static void glb_rr_connectivity_changed(grpc_exec_ctx *exec_ctx, void *arg, /* RR not shutting down. Mimic the RR's policy state */ grpc_connectivity_state_set(exec_ctx, &glb_policy->state_tracker, rr_conn_data->state, GRPC_ERROR_REF(error), - "glb_rr_connectivity_changed"); - /* resubscribe */ + "rr_connectivity_cb"); + /* resubscribe. Reuse the "rr_connectivity_cb" weak ref. */ grpc_lb_policy_notify_on_state_change(exec_ctx, glb_policy->rr_policy, &rr_conn_data->state, &rr_conn_data->on_change); gpr_mu_unlock(&glb_policy->mu); } else { GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, - "rr_connectiviby_cb"); + "rr_connectivity_cb"); gpr_free(rr_conn_data); } } @@ -1010,9 +1010,6 @@ static void lb_call_destroy(glb_lb_policy *glb_policy) { static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, glb_lb_policy *glb_policy) { GPR_ASSERT(glb_policy->lb_channel != NULL); - /* take a weak ref (won't prevent calling of \a glb_shutdown if the strong ref - * count goes to zero) to be unref'd in lb_on_server_status_received */ - GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "query_for_backends"); lb_call_init(glb_policy); if (grpc_lb_glb_trace) { @@ -1056,6 +1053,9 @@ static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, op->flags = 0; op->reserved = NULL; op++; + /* take a weak ref (won't prevent calling of \a glb_shutdown if the strong ref + * count goes to zero) to be unref'd in lb_on_server_status_received */ + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "lb_on_server_status_received"); call_error = grpc_call_start_batch_and_execute( exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), &glb_policy->lb_on_server_status_received); @@ -1067,6 +1067,8 @@ static void query_for_backends_locked(grpc_exec_ctx *exec_ctx, op->flags = 0; op->reserved = NULL; op++; + /* take another weak ref to be unref'd in lb_on_response_received */ + GRPC_LB_POLICY_WEAK_REF(&glb_policy->base, "lb_on_response_received"); call_error = grpc_call_start_batch_and_execute( exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), &glb_policy->lb_on_response_received); @@ -1145,14 +1147,19 @@ static void lb_on_response_received(grpc_exec_ctx *exec_ctx, void *arg, op->flags = 0; op->reserved = NULL; op++; + /* reuse the "lb_on_response_received" weak ref taken in + * query_for_backends_locked() */ const grpc_call_error call_error = grpc_call_start_batch_and_execute( exec_ctx, glb_policy->lb_call, ops, (size_t)(op - ops), &glb_policy->lb_on_response_received); /* loop */ GPR_ASSERT(GRPC_CALL_OK == call_error); } - return; + } else { /* empty payload: call cancelled. */ + /* dispose of the "lb_on_response_received" weak ref taken in + * query_for_backends_locked() and reused in every reception loop */ + GRPC_LB_POLICY_WEAK_UNREF(exec_ctx, &glb_policy->base, + "lb_on_response_received_empty_payload"); } - /* else, empty payload: call cancelled. */ } static void lb_call_on_retry_timer(grpc_exec_ctx *exec_ctx, void *arg, diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 9b20edec92c..4700267ba2a 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -270,7 +270,7 @@ static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { subchannel_data *sd = p->subchannels[i]; GRPC_SUBCHANNEL_UNREF(exec_ctx, sd->subchannel, "round_robin_destroy"); if (sd->user_data != NULL) { - GPR_ASSERT(sd->user_data_vtable); + GPR_ASSERT(sd->user_data_vtable != NULL); sd->user_data_vtable->destroy(sd->user_data); } gpr_free(sd); From d14458f5894444aeaf3793d29d3c3d299493ecd1 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 13:19:40 -0700 Subject: [PATCH 20/28] Reverted back to using 128 bytes for service name --- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c | 9 ++++++++- .../lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h | 6 +++--- src/proto/grpc/lb/v1/load_balancer.options | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c index c90d3f9ff59..afecb716fb0 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.c @@ -106,7 +106,14 @@ PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) #endif #if !defined(PB_FIELD_16BIT) && !defined(PB_FIELD_32BIT) -#error Field descriptor for grpc_lb_v1_InitialLoadBalanceRequest.name is too large. Define PB_FIELD_16BIT to fix this. +/* If you get an error here, it means that you need to define PB_FIELD_16BIT + * compile-time option. You can do that in pb.h or on compiler command line. + * + * The reason you need to do this is that some of your messages contain tag + * numbers or field sizes that are larger than what can fit in the default + * 8 bit descriptors. + */ +PB_STATIC_ASSERT((pb_membersize(grpc_lb_v1_LoadBalanceRequest, initial_request) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceRequest, client_stats) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, initial_response) < 256 && pb_membersize(grpc_lb_v1_LoadBalanceResponse, server_list) < 256 && pb_membersize(grpc_lb_v1_InitialLoadBalanceResponse, client_stats_report_interval) < 256 && pb_membersize(grpc_lb_v1_ServerList, servers) < 256 && pb_membersize(grpc_lb_v1_ServerList, expiration_interval) < 256), YOU_MUST_DEFINE_PB_FIELD_16BIT_FOR_MESSAGES_grpc_lb_v1_Duration_grpc_lb_v1_LoadBalanceRequest_grpc_lb_v1_InitialLoadBalanceRequest_grpc_lb_v1_ClientStats_grpc_lb_v1_LoadBalanceResponse_grpc_lb_v1_InitialLoadBalanceResponse_grpc_lb_v1_ServerList_grpc_lb_v1_Server) #endif diff --git a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h index 81e89254d17..e36d0966f85 100644 --- a/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h +++ b/src/core/ext/lb_policy/grpclb/proto/grpc/lb/v1/load_balancer.pb.h @@ -66,7 +66,7 @@ typedef struct _grpc_lb_v1_Duration { typedef struct _grpc_lb_v1_InitialLoadBalanceRequest { bool has_name; - char name[256]; + char name[128]; /* @@protoc_insertion_point(struct:grpc_lb_v1_InitialLoadBalanceRequest) */ } grpc_lb_v1_InitialLoadBalanceRequest; @@ -166,8 +166,8 @@ extern const pb_field_t grpc_lb_v1_Server_fields[5]; /* Maximum encoded size of messages (where known) */ #define grpc_lb_v1_Duration_size 22 -#define grpc_lb_v1_LoadBalanceRequest_size 297 -#define grpc_lb_v1_InitialLoadBalanceRequest_size 259 +#define grpc_lb_v1_LoadBalanceRequest_size 169 +#define grpc_lb_v1_InitialLoadBalanceRequest_size 131 #define grpc_lb_v1_ClientStats_size 33 #define grpc_lb_v1_LoadBalanceResponse_size (98 + grpc_lb_v1_ServerList_size) #define grpc_lb_v1_InitialLoadBalanceResponse_size 90 diff --git a/src/proto/grpc/lb/v1/load_balancer.options b/src/proto/grpc/lb/v1/load_balancer.options index 46e27fa2c11..7fbd44b9ded 100644 --- a/src/proto/grpc/lb/v1/load_balancer.options +++ b/src/proto/grpc/lb/v1/load_balancer.options @@ -1,4 +1,4 @@ -grpc.lb.v1.InitialLoadBalanceRequest.name max_size:256 +grpc.lb.v1.InitialLoadBalanceRequest.name max_size:128 grpc.lb.v1.InitialLoadBalanceResponse.load_balancer_delegate max_size:64 grpc.lb.v1.Server.ip_address max_size:16 grpc.lb.v1.Server.load_balance_token max_size:50 From c939022dd5b1496c77d6e69a9677b141fc1378fa Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Tue, 1 Nov 2016 15:47:24 -0700 Subject: [PATCH 21/28] More data massaging --- tools/run_tests/performance/bq_upload_result.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tools/run_tests/performance/bq_upload_result.py b/tools/run_tests/performance/bq_upload_result.py index 3a3c4ae7003..0ea23d22122 100755 --- a/tools/run_tests/performance/bq_upload_result.py +++ b/tools/run_tests/performance/bq_upload_result.py @@ -122,6 +122,8 @@ def _flatten_result_inplace(scenario_result): scenario_result['clientSuccess'] = json.dumps(scenario_result['clientSuccess']) scenario_result['serverSuccess'] = json.dumps(scenario_result['serverSuccess']) scenario_result['requestResults'] = json.dumps(scenario_result.get('requestResults', [])) + scenario_result['summary'].pop('successfulRequestsPerSecond', None) + scenario_result['summary'].pop('failedRequestsPerSecond', None) def _populate_metadata_inplace(scenario_result): From e3beac9eebadf0a291664f65cc8cdbe8182150d4 Mon Sep 17 00:00:00 2001 From: Matt Kwong Date: Tue, 1 Nov 2016 12:53:04 -0700 Subject: [PATCH 22/28] Disable gcc4.4 C++ tests - DO NOT MERGE INTO V1.0.X --- tools/run_tests/run_tests_matrix.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/tools/run_tests/run_tests_matrix.py b/tools/run_tests/run_tests_matrix.py index 2656f1ac5dc..41db67cdb58 100755 --- a/tools/run_tests/run_tests_matrix.py +++ b/tools/run_tests/run_tests_matrix.py @@ -181,7 +181,17 @@ def _create_portability_test_jobs(extra_args=[], inner_jobs=_DEFAULT_INNER_JOBS) # portability C and C++ on x64 for compiler in ['gcc4.4', 'gcc4.6', 'gcc5.3', 'clang3.5', 'clang3.6', 'clang3.7']: - test_jobs += _generate_jobs(languages=['c', 'c++'], + test_jobs += _generate_jobs(languages=['c'], + configs=['dbg'], + platforms=['linux'], + arch='x64', + compiler=compiler, + labels=['portability'], + extra_args=extra_args, + inner_jobs=inner_jobs) + for compiler in ['gcc4.8', 'gcc5.3', + 'clang3.5', 'clang3.6', 'clang3.7']: + test_jobs += _generate_jobs(languages=['c++'], configs=['dbg'], platforms=['linux'], arch='x64', From da050b643e302db24d840f02c475655c01182333 Mon Sep 17 00:00:00 2001 From: murgatroid99 Date: Tue, 1 Nov 2016 17:17:28 -0700 Subject: [PATCH 23/28] Fixed weird whitespace change --- src/core/lib/iomgr/tcp_uv.c | 62 ++++++++++++++++++------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/src/core/lib/iomgr/tcp_uv.c b/src/core/lib/iomgr/tcp_uv.c index b90e0c80082..8e74c9e8633 100644 --- a/src/core/lib/iomgr/tcp_uv.c +++ b/src/core/lib/iomgr/tcp_uv.c @@ -1,35 +1,35 @@ /* -* -* Copyright 2016, Google Inc. -* All rights reserved. -* -* Redistribution and use in source and binary forms, with or without -* modification, are permitted provided that the following conditions are -* met: -* -* * Redistributions of source code must retain the above copyright -* notice, this list of conditions and the following disclaimer. -* * Redistributions in binary form must reproduce the above -* copyright notice, this list of conditions and the following disclaimer -* in the documentation and/or other materials provided with the -* distribution. -* * Neither the name of Google Inc. nor the names of its -* contributors may be used to endorse or promote products derived from -* this software without specific prior written permission. -* -* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS -* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT -* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR -* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT -* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, -* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT -* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, -* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY -* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT -* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE -* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. -* -*/ + * + * Copyright 2016, Google Inc. + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * * Redistributions in binary form must reproduce the above + * copyright notice, this list of conditions and the following disclaimer + * in the documentation and/or other materials provided with the + * distribution. + * * Neither the name of Google Inc. nor the names of its + * contributors may be used to endorse or promote products derived from + * this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR + * A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT + * OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE + * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. + * + */ #include "src/core/lib/iomgr/port.h" From 2ff8180b33dfa0fc4191d2608cae4e08860543df Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 21:58:16 -0700 Subject: [PATCH 24/28] clang-format --- src/core/ext/lb_policy/round_robin/round_robin.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index 4700267ba2a..b0c461730b3 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -263,7 +263,7 @@ static void rr_destroy(grpc_exec_ctx *exec_ctx, grpc_lb_policy *pol) { ready_list *elem; if (grpc_lb_round_robin_trace) { - gpr_log(GPR_DEBUG, "Destroying Round Robin policy at %p", (void*)pol); + gpr_log(GPR_DEBUG, "Destroying Round Robin policy at %p", (void *)pol); } for (size_t i = 0; i < p->num_subchannels; i++) { From 6e7baf75dd8ab88d452e7001c41699857afd6e98 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Tue, 1 Nov 2016 23:08:36 -0700 Subject: [PATCH 25/28] generate projects --- tools/run_tests/tests.json | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 733bfe3b8fd..39daf859b46 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -3003,6 +3003,7 @@ ], "cpu_cost": 1.0, "exclude_configs": [], + "exclude_iomgrs": [], "flaky": false, "gtest": false, "language": "c++", From 754b2b88bc7bf35629573ba46a9600e385cd7100 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Wed, 2 Nov 2016 08:39:16 -0700 Subject: [PATCH 26/28] Code review changes. --- include/grpc++/support/channel_arguments.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/include/grpc++/support/channel_arguments.h b/include/grpc++/support/channel_arguments.h index 11d10f4a559..49a3e9461cf 100644 --- a/include/grpc++/support/channel_arguments.h +++ b/include/grpc++/support/channel_arguments.h @@ -85,9 +85,9 @@ class ChannelArguments { /// The given buffer pool will be attached to the constructed channel void SetResourceQuota(const ResourceQuota& resource_quota); - // Set LB policy name. - // Note that if the name resolver returns only balancer addresses, the - // grpclb LB policy will be used, regardless of what is specified here. + /// Set LB policy name. + /// Note that if the name resolver returns only balancer addresses, the + /// grpclb LB policy will be used, regardless of what is specified here. void SetLoadBalancingPolicyName(const grpc::string& lb_policy_name); // Generic channel argument setters. Only for advanced use cases. From 029ed106c5780fe94f18b46e433abaa1f5e603b7 Mon Sep 17 00:00:00 2001 From: Matt Kwong Date: Tue, 1 Nov 2016 18:04:47 -0700 Subject: [PATCH 27/28] Enable port tests to be run using gcc4.8 --- .../dockerfile/test/cxx_ubuntu1604_x64/Dockerfile.template | 2 +- tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile | 2 +- tools/run_tests/run_tests.py | 4 +++- tools/run_tests/tests.json | 1 + 4 files changed, 6 insertions(+), 3 deletions(-) diff --git a/templates/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile.template b/templates/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile.template index 42ad6c130da..2d780d74f88 100644 --- a/templates/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile.template +++ b/templates/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile.template @@ -39,7 +39,7 @@ # The clang-3.6 symlink for the default clang version was added # to Ubuntu 16.04 recently, so make sure it's installed. # Also install clang3.7. - RUN apt-get update && apt-get -y install clang-3.6 clang-3.7 && apt-get clean + RUN apt-get update && apt-get -y install gcc-4.8 gcc-4.8-multilib g++-4.8 g++-4.8-multilib clang-3.6 clang-3.7 && apt-get clean # Define the default command. CMD ["bash"] diff --git a/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile b/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile index 2d282276d31..b4847124cfc 100644 --- a/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile +++ b/tools/dockerfile/test/cxx_ubuntu1604_x64/Dockerfile @@ -97,7 +97,7 @@ RUN mkdir /var/local/jenkins # The clang-3.6 symlink for the default clang version was added # to Ubuntu 16.04 recently, so make sure it's installed. # Also install clang3.7. -RUN apt-get update && apt-get -y install clang-3.6 clang-3.7 && apt-get clean +RUN apt-get update && apt-get -y install gcc-4.8 gcc-4.8-multilib g++-4.8 g++-4.8-multilib clang-3.6 clang-3.7 && apt-get clean # Define the default command. CMD ["bash"] diff --git a/tools/run_tests/run_tests.py b/tools/run_tests/run_tests.py index 911843e9f36..05f819bd9bb 100755 --- a/tools/run_tests/run_tests.py +++ b/tools/run_tests/run_tests.py @@ -332,6 +332,8 @@ class CLanguage(object): return ('wheezy', self._gcc_make_options(version_suffix='-4.4')) elif compiler == 'gcc4.6': return ('wheezy', self._gcc_make_options(version_suffix='-4.6')) + elif compiler == 'gcc4.8': + return ('ubuntu1604', self._gcc_make_options(version_suffix='-4.8')) elif compiler == 'gcc5.3': return ('ubuntu1604', []) elif compiler == 'clang3.4': @@ -1061,7 +1063,7 @@ argp.add_argument('--arch', help='Selects architecture to target. For some platforms "default" is the only supported choice.') argp.add_argument('--compiler', choices=['default', - 'gcc4.4', 'gcc4.6', 'gcc4.9', 'gcc5.3', + 'gcc4.4', 'gcc4.6', 'gcc4.8', 'gcc4.9', 'gcc5.3', 'clang3.4', 'clang3.5', 'clang3.6', 'clang3.7', 'vs2010', 'vs2013', 'vs2015', 'python2.7', 'python3.4', 'python3.5', 'python3.6', 'pypy', 'pypy3', diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 733bfe3b8fd..39daf859b46 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -3003,6 +3003,7 @@ ], "cpu_cost": 1.0, "exclude_configs": [], + "exclude_iomgrs": [], "flaky": false, "gtest": false, "language": "c++", From 5bdcd237fc907eb4dc334e89d8ae2fa00c0f2720 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Wed, 2 Nov 2016 15:47:02 -0700 Subject: [PATCH 28/28] RR: Don't copy user_data is no vtable --- src/core/ext/lb_policy/round_robin/round_robin.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/core/ext/lb_policy/round_robin/round_robin.c b/src/core/ext/lb_policy/round_robin/round_robin.c index b0c461730b3..427999aa6b8 100644 --- a/src/core/ext/lb_policy/round_robin/round_robin.c +++ b/src/core/ext/lb_policy/round_robin/round_robin.c @@ -678,8 +678,10 @@ static grpc_lb_policy *round_robin_create(grpc_exec_ctx *exec_ctx, sd->index = subchannel_idx; sd->subchannel = subchannel; sd->user_data_vtable = addresses->user_data_vtable; - sd->user_data = - sd->user_data_vtable->copy(addresses->addresses[i].user_data); + if (sd->user_data_vtable != NULL) { + sd->user_data = + sd->user_data_vtable->copy(addresses->addresses[i].user_data); + } ++subchannel_idx; grpc_closure_init(&sd->connectivity_changed_closure, rr_connectivity_changed, sd);