From 57e2743b6580ae19326ceccb5ac9ed88e0691a94 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 16:53:58 -0800 Subject: [PATCH 1/9] Add contributed test --- Makefile | 36 ++++ build.yaml | 10 + .../surface/concurrent_connectivity_test.c | 48 +++++ tools/run_tests/sources_and_headers.json | 16 ++ tools/run_tests/tests.json | 21 ++ vsprojects/buildtests_c.sln | 27 +++ .../concurrent_connectivity_test.vcxproj | 199 ++++++++++++++++++ ...ncurrent_connectivity_test.vcxproj.filters | 21 ++ 8 files changed, 378 insertions(+) create mode 100644 test/core/surface/concurrent_connectivity_test.c create mode 100644 vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj create mode 100644 vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj.filters diff --git a/Makefile b/Makefile index 171a83c2a3a..f118d542538 100644 --- a/Makefile +++ b/Makefile @@ -849,6 +849,7 @@ chttp2_status_conversion_test: $(BINDIR)/$(CONFIG)/chttp2_status_conversion_test chttp2_stream_map_test: $(BINDIR)/$(CONFIG)/chttp2_stream_map_test chttp2_varint_test: $(BINDIR)/$(CONFIG)/chttp2_varint_test compression_test: $(BINDIR)/$(CONFIG)/compression_test +concurrent_connectivity_test: $(BINDIR)/$(CONFIG)/concurrent_connectivity_test dns_resolver_connectivity_test: $(BINDIR)/$(CONFIG)/dns_resolver_connectivity_test dns_resolver_test: $(BINDIR)/$(CONFIG)/dns_resolver_test dualstack_socket_test: $(BINDIR)/$(CONFIG)/dualstack_socket_test @@ -1160,6 +1161,7 @@ buildtests_c: privatelibs_c \ $(BINDIR)/$(CONFIG)/chttp2_stream_map_test \ $(BINDIR)/$(CONFIG)/chttp2_varint_test \ $(BINDIR)/$(CONFIG)/compression_test \ + $(BINDIR)/$(CONFIG)/concurrent_connectivity_test \ $(BINDIR)/$(CONFIG)/dns_resolver_connectivity_test \ $(BINDIR)/$(CONFIG)/dns_resolver_test \ $(BINDIR)/$(CONFIG)/dualstack_socket_test \ @@ -1404,6 +1406,8 @@ test_c: buildtests_c $(Q) $(BINDIR)/$(CONFIG)/chttp2_varint_test || ( echo test chttp2_varint_test failed ; exit 1 ) $(E) "[RUN] Testing compression_test" $(Q) $(BINDIR)/$(CONFIG)/compression_test || ( echo test compression_test failed ; exit 1 ) + $(E) "[RUN] Testing concurrent_connectivity_test" + $(Q) $(BINDIR)/$(CONFIG)/concurrent_connectivity_test || ( echo test concurrent_connectivity_test failed ; exit 1 ) $(E) "[RUN] Testing dns_resolver_connectivity_test" $(Q) $(BINDIR)/$(CONFIG)/dns_resolver_connectivity_test || ( echo test dns_resolver_connectivity_test failed ; exit 1 ) $(E) "[RUN] Testing dns_resolver_test" @@ -6079,6 +6083,38 @@ endif endif +CONCURRENT_CONNECTIVITY_TEST_SRC = \ + test/core/surface/concurrent_connectivity_test.c \ + +CONCURRENT_CONNECTIVITY_TEST_OBJS = $(addprefix $(OBJDIR)/$(CONFIG)/, $(addsuffix .o, $(basename $(CONCURRENT_CONNECTIVITY_TEST_SRC)))) +ifeq ($(NO_SECURE),true) + +# You can't build secure targets if you don't have OpenSSL. + +$(BINDIR)/$(CONFIG)/concurrent_connectivity_test: openssl_dep_error + +else + + + +$(BINDIR)/$(CONFIG)/concurrent_connectivity_test: $(CONCURRENT_CONNECTIVITY_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + $(E) "[LD] Linking $@" + $(Q) mkdir -p `dirname $@` + $(Q) $(LD) $(LDFLAGS) $(CONCURRENT_CONNECTIVITY_TEST_OBJS) $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a $(LDLIBS) $(LDLIBS_SECURE) -o $(BINDIR)/$(CONFIG)/concurrent_connectivity_test + +endif + +$(OBJDIR)/$(CONFIG)/test/core/surface/concurrent_connectivity_test.o: $(LIBDIR)/$(CONFIG)/libgrpc_test_util.a $(LIBDIR)/$(CONFIG)/libgrpc.a $(LIBDIR)/$(CONFIG)/libgpr_test_util.a $(LIBDIR)/$(CONFIG)/libgpr.a + +deps_concurrent_connectivity_test: $(CONCURRENT_CONNECTIVITY_TEST_OBJS:.o=.dep) + +ifneq ($(NO_SECURE),true) +ifneq ($(NO_DEPS),true) +-include $(CONCURRENT_CONNECTIVITY_TEST_OBJS:.o=.dep) +endif +endif + + DNS_RESOLVER_CONNECTIVITY_TEST_SRC = \ test/core/client_config/resolvers/dns_resolver_connectivity_test.c \ diff --git a/build.yaml b/build.yaml index e003dd83396..83b7714e655 100644 --- a/build.yaml +++ b/build.yaml @@ -1057,6 +1057,16 @@ targets: - grpc - gpr_test_util - gpr +- name: concurrent_connectivity_test + build: test + language: c + src: + - test/core/surface/concurrent_connectivity_test.c + deps: + - grpc_test_util + - grpc + - gpr_test_util + - gpr - name: dns_resolver_connectivity_test cpu_cost: 0.1 build: test diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c new file mode 100644 index 00000000000..d8639f5e510 --- /dev/null +++ b/test/core/surface/concurrent_connectivity_test.c @@ -0,0 +1,48 @@ +#include + +#include +#include +#include + +#define NUM_THREADS 100 +static grpc_channel* channels[NUM_THREADS]; +static grpc_completion_queue* queues[NUM_THREADS]; + +void create_loop_destroy(void* actually_an_int) { + int thread_index = (int)(actually_an_int); + for (int i = 0; i < 10; ++i) { + grpc_completion_queue* cq = grpc_completion_queue_create(NULL); + grpc_channel* chan = grpc_insecure_channel_create("localhost", NULL, NULL); + + channels[thread_index] = chan; + queues[thread_index] = cq; + + gpr_timespec inf_future = gpr_inf_future(GPR_CLOCK_REALTIME); + gpr_timespec delta = gpr_time_from_millis(10, GPR_TIMESPAN); + for (int j = 0; j < 10; ++j) { + gpr_timespec later_time = + gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), delta); + grpc_connectivity_state state = + grpc_channel_check_connectivity_state(chan, 1); + grpc_channel_watch_connectivity_state(chan, state, later_time, cq, NULL); + grpc_completion_queue_next(cq, inf_future, NULL); + } + grpc_channel_destroy(channels[thread_index]); + grpc_completion_queue_destroy(queues[thread_index]); + } +} + +int main() { + grpc_init(); + gpr_thd_id threads[NUM_THREADS]; + for (intptr_t i = 0; i < NUM_THREADS; ++i) { + gpr_thd_options options = gpr_thd_options_default(); + gpr_thd_options_set_joinable(&options); + gpr_thd_new(&threads[i], create_loop_destroy, (void*)i, &options); + } + for (int i = 0; i < NUM_THREADS; ++i) { + gpr_thd_join(threads[i]); + } + grpc_shutdown(); + return 0; +} diff --git a/tools/run_tests/sources_and_headers.json b/tools/run_tests/sources_and_headers.json index 0039b99a55e..092ed35ad97 100644 --- a/tools/run_tests/sources_and_headers.json +++ b/tools/run_tests/sources_and_headers.json @@ -189,6 +189,22 @@ "third_party": false, "type": "target" }, + { + "deps": [ + "gpr", + "gpr_test_util", + "grpc", + "grpc_test_util" + ], + "headers": [], + "language": "c", + "name": "concurrent_connectivity_test", + "src": [ + "test/core/surface/concurrent_connectivity_test.c" + ], + "third_party": false, + "type": "target" + }, { "deps": [ "gpr", diff --git a/tools/run_tests/tests.json b/tools/run_tests/tests.json index 724cd64d19c..000315f2f4a 100644 --- a/tools/run_tests/tests.json +++ b/tools/run_tests/tests.json @@ -253,6 +253,27 @@ "windows" ] }, + { + "args": [], + "ci_platforms": [ + "linux", + "mac", + "posix", + "windows" + ], + "cpu_cost": 1.0, + "exclude_configs": [], + "flaky": false, + "gtest": false, + "language": "c", + "name": "concurrent_connectivity_test", + "platforms": [ + "linux", + "mac", + "posix", + "windows" + ] + }, { "args": [], "ci_platforms": [ diff --git a/vsprojects/buildtests_c.sln b/vsprojects/buildtests_c.sln index be2c4a3bb14..413ed3e3f38 100644 --- a/vsprojects/buildtests_c.sln +++ b/vsprojects/buildtests_c.sln @@ -251,6 +251,17 @@ Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "compression_test", "vcxproj {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} = {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} EndProjectSection EndProject +Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "concurrent_connectivity_test", "vcxproj\test\concurrent_connectivity_test\concurrent_connectivity_test.vcxproj", "{391B366C-D916-45AA-3FE5-67363A46193B}" + ProjectSection(myProperties) = preProject + lib = "False" + EndProjectSection + ProjectSection(ProjectDependencies) = postProject + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} = {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + {29D16885-7228-4C31-81ED-5F9187C7F2A9} = {29D16885-7228-4C31-81ED-5F9187C7F2A9} + {EAB0A629-17A9-44DB-B5FF-E91A721FE037} = {EAB0A629-17A9-44DB-B5FF-E91A721FE037} + {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} = {B23D3D1A-9438-4EDA-BEB6-9A0A03D17792} + EndProjectSection +EndProject Project("{8BC9CEB8-8B4A-11D0-8D11-00A0C91BC942}") = "dns_resolver_connectivity_test", "vcxproj\test\dns_resolver_connectivity_test\dns_resolver_connectivity_test.vcxproj", "{F7B6FE68-E847-D7CA-4062-E737E542BCC3}" ProjectSection(myProperties) = preProject lib = "False" @@ -1747,6 +1758,22 @@ Global {5AFE7D17-A4A7-D68E-4491-CBC852F9D2A0}.Release-DLL|Win32.Build.0 = Release|Win32 {5AFE7D17-A4A7-D68E-4491-CBC852F9D2A0}.Release-DLL|x64.ActiveCfg = Release|x64 {5AFE7D17-A4A7-D68E-4491-CBC852F9D2A0}.Release-DLL|x64.Build.0 = Release|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug|Win32.ActiveCfg = Debug|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug|x64.ActiveCfg = Debug|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release|Win32.ActiveCfg = Release|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release|x64.ActiveCfg = Release|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug|Win32.Build.0 = Debug|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug|x64.Build.0 = Debug|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release|Win32.Build.0 = Release|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release|x64.Build.0 = Release|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug-DLL|Win32.ActiveCfg = Debug|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug-DLL|Win32.Build.0 = Debug|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug-DLL|x64.ActiveCfg = Debug|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Debug-DLL|x64.Build.0 = Debug|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release-DLL|Win32.ActiveCfg = Release|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release-DLL|Win32.Build.0 = Release|Win32 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release-DLL|x64.ActiveCfg = Release|x64 + {391B366C-D916-45AA-3FE5-67363A46193B}.Release-DLL|x64.Build.0 = Release|x64 {F7B6FE68-E847-D7CA-4062-E737E542BCC3}.Debug|Win32.ActiveCfg = Debug|Win32 {F7B6FE68-E847-D7CA-4062-E737E542BCC3}.Debug|x64.ActiveCfg = Debug|x64 {F7B6FE68-E847-D7CA-4062-E737E542BCC3}.Release|Win32.ActiveCfg = Release|Win32 diff --git a/vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj b/vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj new file mode 100644 index 00000000000..6d69eae302f --- /dev/null +++ b/vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj @@ -0,0 +1,199 @@ + + + + + + Debug + Win32 + + + Debug + x64 + + + Release + Win32 + + + Release + x64 + + + + {391B366C-D916-45AA-3FE5-67363A46193B} + true + $(SolutionDir)IntDir\$(MSBuildProjectName)\ + + + + v100 + + + v110 + + + v120 + + + v140 + + + Application + true + Unicode + + + Application + false + true + Unicode + + + + + + + + + + + + + + concurrent_connectivity_test + static + Debug + static + Debug + + + concurrent_connectivity_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 + + + + + + + + + + {17BCAFC0-5FDC-4C94-AEB9-95F3E220614B} + + + {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/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj.filters b/vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj.filters new file mode 100644 index 00000000000..a6492c775a2 --- /dev/null +++ b/vsprojects/vcxproj/test/concurrent_connectivity_test/concurrent_connectivity_test.vcxproj.filters @@ -0,0 +1,21 @@ + + + + + test\core\surface + + + + + + {1320b717-a107-004b-c517-074e6a7baf97} + + + {f205219c-cee2-c0e8-a922-5486f2d31026} + + + {6804b6ae-88f4-acf9-b3fc-6b12f8e28d4d} + + + + From 0ce7bec923867aae3b115a9a7e4fc2e56f021353 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 17:02:07 -0800 Subject: [PATCH 2/9] Fail faster --- test/core/surface/concurrent_connectivity_test.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index d8639f5e510..1046d422774 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -2,7 +2,9 @@ #include #include +#include #include +#include "test/core/util/test_config.h" #define NUM_THREADS 100 static grpc_channel* channels[NUM_THREADS]; @@ -17,22 +19,22 @@ void create_loop_destroy(void* actually_an_int) { channels[thread_index] = chan; queues[thread_index] = cq; - gpr_timespec inf_future = gpr_inf_future(GPR_CLOCK_REALTIME); - gpr_timespec delta = gpr_time_from_millis(10, GPR_TIMESPAN); for (int j = 0; j < 10; ++j) { - gpr_timespec later_time = - gpr_time_add(gpr_now(GPR_CLOCK_REALTIME), delta); + gpr_timespec later_time = GRPC_TIMEOUT_MILLIS_TO_DEADLINE(10); grpc_connectivity_state state = grpc_channel_check_connectivity_state(chan, 1); grpc_channel_watch_connectivity_state(chan, state, later_time, cq, NULL); - grpc_completion_queue_next(cq, inf_future, NULL); + GPR_ASSERT(grpc_completion_queue_next( + cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(3), NULL) + .type == GRPC_OP_COMPLETE); } grpc_channel_destroy(channels[thread_index]); grpc_completion_queue_destroy(queues[thread_index]); } } -int main() { +int main(int argc, char** argv) { + grpc_test_init(argc, argv); grpc_init(); gpr_thd_id threads[NUM_THREADS]; for (intptr_t i = 0; i < NUM_THREADS; ++i) { From 2ba4133d3a260f3fe218160ef6fe68e0edc1e4a7 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 17:09:12 -0800 Subject: [PATCH 3/9] Fix race in subchannel.c --- src/core/client_config/subchannel.c | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/src/core/client_config/subchannel.c b/src/core/client_config/subchannel.c index d91dd116b8c..ddd129c5397 100644 --- a/src/core/client_config/subchannel.c +++ b/src/core/client_config/subchannel.c @@ -184,8 +184,8 @@ static void connection_destroy(grpc_exec_ctx *exec_ctx, void *arg, gpr_free(c); } -void grpc_connected_subchannel_ref(grpc_connected_subchannel *c - GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +void grpc_connected_subchannel_ref( + grpc_connected_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { GRPC_CHANNEL_STACK_REF(CHANNEL_STACK_FROM_CONNECTION(c), REF_REASON); } @@ -226,8 +226,8 @@ static gpr_atm ref_mutate(grpc_subchannel *c, gpr_atm delta, return old_val; } -grpc_subchannel *grpc_subchannel_ref(grpc_subchannel *c - GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +grpc_subchannel *grpc_subchannel_ref( + grpc_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { gpr_atm old_refs; old_refs = ref_mutate(c, (1 << INTERNAL_REF_BITS), 0 REF_MUTATE_PURPOSE("STRONG_REF")); @@ -235,8 +235,8 @@ grpc_subchannel *grpc_subchannel_ref(grpc_subchannel *c return c; } -grpc_subchannel *grpc_subchannel_weak_ref(grpc_subchannel *c - GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +grpc_subchannel *grpc_subchannel_weak_ref( + grpc_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { gpr_atm old_refs; old_refs = ref_mutate(c, 1, 0 REF_MUTATE_PURPOSE("WEAK_REF")); GPR_ASSERT(old_refs != 0); @@ -646,19 +646,23 @@ static void subchannel_connected(grpc_exec_ctx *exec_ctx, void *arg, if (c->connecting_result.transport != NULL) { publish_transport(exec_ctx, c); - } else if (c->disconnected) { + } + + GRPC_SUBCHANNEL_WEAK_REF(c, "connected"); + gpr_mu_lock(&c->mu); + if (c->disconnected) { GRPC_SUBCHANNEL_WEAK_UNREF(exec_ctx, c, "connecting"); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); - gpr_mu_lock(&c->mu); GPR_ASSERT(!c->have_alarm); c->have_alarm = 1; grpc_connectivity_state_set(exec_ctx, &c->state_tracker, GRPC_CHANNEL_TRANSIENT_FAILURE, "connect_failed"); grpc_timer_init(exec_ctx, &c->alarm, c->next_attempt, on_alarm, c, now); - gpr_mu_unlock(&c->mu); } + gpr_mu_unlock(&c->mu); + GRPC_SUBCHANNEL_WEAK_UNREF(exec_ctx, c, "connecting"); } static gpr_timespec compute_connect_deadline(grpc_subchannel *c) { @@ -686,8 +690,8 @@ static void subchannel_call_destroy(grpc_exec_ctx *exec_ctx, void *call, GPR_TIMER_END("grpc_subchannel_call_unref.destroy", 0); } -void grpc_subchannel_call_ref(grpc_subchannel_call *c - GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +void grpc_subchannel_call_ref( + grpc_subchannel_call *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { GRPC_CALL_STACK_REF(SUBCHANNEL_CALL_TO_CALL_STACK(c), REF_REASON); } From 64baf2cbab3bc3caa98907758ee0a6c7db72e68a Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 17:16:33 -0800 Subject: [PATCH 4/9] Extend timeout --- test/core/surface/concurrent_connectivity_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index 1046d422774..7306a394ebd 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -25,7 +25,7 @@ void create_loop_destroy(void* actually_an_int) { grpc_channel_check_connectivity_state(chan, 1); grpc_channel_watch_connectivity_state(chan, state, later_time, cq, NULL); GPR_ASSERT(grpc_completion_queue_next( - cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(3), NULL) + cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(30), NULL) .type == GRPC_OP_COMPLETE); } grpc_channel_destroy(channels[thread_index]); From d24fc85b59b4224d7932c5a7a9f17e73402f388b Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 17:18:19 -0800 Subject: [PATCH 5/9] Fix cast --- test/core/surface/concurrent_connectivity_test.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index 7306a394ebd..cd1aead5e51 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -11,7 +11,7 @@ static grpc_channel* channels[NUM_THREADS]; static grpc_completion_queue* queues[NUM_THREADS]; void create_loop_destroy(void* actually_an_int) { - int thread_index = (int)(actually_an_int); + int thread_index = (int)(intptr_t)(actually_an_int); for (int i = 0; i < 10; ++i) { grpc_completion_queue* cq = grpc_completion_queue_create(NULL); grpc_channel* chan = grpc_insecure_channel_create("localhost", NULL, NULL); From 3591482a372dee047a7c0867ada8cc8d360d247d Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 19:29:23 -0800 Subject: [PATCH 6/9] Expand lock --- src/core/client_config/subchannel.c | 14 +++++--------- test/core/surface/concurrent_connectivity_test.c | 2 +- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/src/core/client_config/subchannel.c b/src/core/client_config/subchannel.c index ddd129c5397..e8f308b6076 100644 --- a/src/core/client_config/subchannel.c +++ b/src/core/client_config/subchannel.c @@ -505,7 +505,8 @@ void grpc_connected_subchannel_ping(grpc_exec_ctx *exec_ctx, elem->filter->start_transport_op(exec_ctx, elem, &op); } -static void publish_transport(grpc_exec_ctx *exec_ctx, grpc_subchannel *c) { +static void publish_transport_locked(grpc_exec_ctx *exec_ctx, + grpc_subchannel *c) { size_t channel_stack_size; grpc_connected_subchannel *con; grpc_channel_stack *stk; @@ -541,8 +542,6 @@ static void publish_transport(grpc_exec_ctx *exec_ctx, grpc_subchannel *c) { grpc_closure_init(&sw_subchannel->closure, subchannel_on_child_state_changed, sw_subchannel); - gpr_mu_lock(&c->mu); - if (c->disconnected) { gpr_mu_unlock(&c->mu); gpr_free(sw_subchannel); @@ -575,7 +574,6 @@ static void publish_transport(grpc_exec_ctx *exec_ctx, grpc_subchannel *c) { grpc_connectivity_state_set(exec_ctx, &c->state_tracker, GRPC_CHANNEL_READY, "connected"); - gpr_mu_unlock(&c->mu); gpr_free((void *)filters); } @@ -644,13 +642,11 @@ static void subchannel_connected(grpc_exec_ctx *exec_ctx, void *arg, bool iomgr_success) { grpc_subchannel *c = arg; - if (c->connecting_result.transport != NULL) { - publish_transport(exec_ctx, c); - } - GRPC_SUBCHANNEL_WEAK_REF(c, "connected"); gpr_mu_lock(&c->mu); - if (c->disconnected) { + if (c->connecting_result.transport != NULL) { + publish_transport_locked(exec_ctx, c); + } else if (c->disconnected) { GRPC_SUBCHANNEL_WEAK_UNREF(exec_ctx, c, "connecting"); } else { gpr_timespec now = gpr_now(GPR_CLOCK_MONOTONIC); diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index 7306a394ebd..1046d422774 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -25,7 +25,7 @@ void create_loop_destroy(void* actually_an_int) { grpc_channel_check_connectivity_state(chan, 1); grpc_channel_watch_connectivity_state(chan, state, later_time, cq, NULL); GPR_ASSERT(grpc_completion_queue_next( - cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(30), NULL) + cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(3), NULL) .type == GRPC_OP_COMPLETE); } grpc_channel_destroy(channels[thread_index]); From 9fb89ea9e31dbe376fc1ce9c173d4e52f4b31290 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 20:12:45 -0800 Subject: [PATCH 7/9] If we cant check timers due to contention, at least make sure we do a follow up check soon --- src/core/client_config/subchannel.c | 16 ++++---- src/core/iomgr/timer.c | 3 ++ src/core/iomgr/timer.h | 1 - src/core/surface/completion_queue.c | 2 +- .../surface/concurrent_connectivity_test.c | 39 +++++++++++++++++-- test/core/surface/lame_client_test.c | 2 +- 6 files changed, 49 insertions(+), 14 deletions(-) diff --git a/src/core/client_config/subchannel.c b/src/core/client_config/subchannel.c index e8f308b6076..ec9e47a6ddb 100644 --- a/src/core/client_config/subchannel.c +++ b/src/core/client_config/subchannel.c @@ -184,8 +184,8 @@ static void connection_destroy(grpc_exec_ctx *exec_ctx, void *arg, gpr_free(c); } -void grpc_connected_subchannel_ref( - grpc_connected_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +void grpc_connected_subchannel_ref(grpc_connected_subchannel *c + GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { GRPC_CHANNEL_STACK_REF(CHANNEL_STACK_FROM_CONNECTION(c), REF_REASON); } @@ -226,8 +226,8 @@ static gpr_atm ref_mutate(grpc_subchannel *c, gpr_atm delta, return old_val; } -grpc_subchannel *grpc_subchannel_ref( - grpc_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +grpc_subchannel *grpc_subchannel_ref(grpc_subchannel *c + GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { gpr_atm old_refs; old_refs = ref_mutate(c, (1 << INTERNAL_REF_BITS), 0 REF_MUTATE_PURPOSE("STRONG_REF")); @@ -235,8 +235,8 @@ grpc_subchannel *grpc_subchannel_ref( return c; } -grpc_subchannel *grpc_subchannel_weak_ref( - grpc_subchannel *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +grpc_subchannel *grpc_subchannel_weak_ref(grpc_subchannel *c + GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { gpr_atm old_refs; old_refs = ref_mutate(c, 1, 0 REF_MUTATE_PURPOSE("WEAK_REF")); GPR_ASSERT(old_refs != 0); @@ -686,8 +686,8 @@ static void subchannel_call_destroy(grpc_exec_ctx *exec_ctx, void *call, GPR_TIMER_END("grpc_subchannel_call_unref.destroy", 0); } -void grpc_subchannel_call_ref( - grpc_subchannel_call *c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { +void grpc_subchannel_call_ref(grpc_subchannel_call *c + GRPC_SUBCHANNEL_REF_EXTRA_ARGS) { GRPC_CALL_STACK_REF(SUBCHANNEL_CALL_TO_CALL_STACK(c), REF_REASON); } diff --git a/src/core/iomgr/timer.c b/src/core/iomgr/timer.c index 8379fffad02..8badd83bbb1 100644 --- a/src/core/iomgr/timer.c +++ b/src/core/iomgr/timer.c @@ -330,6 +330,9 @@ static int run_some_expired_timers(grpc_exec_ctx *exec_ctx, gpr_timespec now, gpr_mu_unlock(&g_mu); gpr_mu_unlock(&g_checker_mu); + } else if (next != NULL) { + *next = gpr_time_min( + *next, gpr_time_add(now, gpr_time_from_millis(100, GPR_TIMESPAN))); } return (int)n; diff --git a/src/core/iomgr/timer.h b/src/core/iomgr/timer.h index 9ad1e92f42e..e239e884e7b 100644 --- a/src/core/iomgr/timer.h +++ b/src/core/iomgr/timer.h @@ -96,7 +96,6 @@ void grpc_timer_cancel(grpc_exec_ctx *exec_ctx, grpc_timer *timer); *next is never guaranteed to be updated on any given execution; however, with high probability at least one thread in the system will see an update at any time slice. */ - bool grpc_timer_check(grpc_exec_ctx *exec_ctx, gpr_timespec now, gpr_timespec *next); void grpc_timer_list_init(gpr_timespec now); diff --git a/src/core/surface/completion_queue.c b/src/core/surface/completion_queue.c index f6a95ebbd3f..b22818ea87d 100644 --- a/src/core/surface/completion_queue.c +++ b/src/core/surface/completion_queue.c @@ -86,7 +86,7 @@ struct grpc_completion_queue { #define POLLSET_FROM_CQ(cq) ((grpc_pollset *)(cq + 1)) static gpr_mu g_freelist_mu; -grpc_completion_queue *g_freelist; +static grpc_completion_queue *g_freelist; static void on_pollset_shutdown_done(grpc_exec_ctx *exec_ctx, void *cc, bool success); diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index b380d4471fc..5361201db72 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -1,3 +1,36 @@ +/* +* +* Copyright 2015-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 @@ -24,9 +57,9 @@ void create_loop_destroy(void* actually_an_int) { grpc_connectivity_state state = grpc_channel_check_connectivity_state(chan, 1); grpc_channel_watch_connectivity_state(chan, state, later_time, cq, NULL); - GPR_ASSERT(grpc_completion_queue_next( - cq, GRPC_TIMEOUT_SECONDS_TO_DEADLINE(3), NULL) - .type == GRPC_OP_COMPLETE); + GPR_ASSERT(grpc_completion_queue_next(cq, + GRPC_TIMEOUT_SECONDS_TO_DEADLINE(3), + NULL).type == GRPC_OP_COMPLETE); } grpc_channel_destroy(channels[thread_index]); grpc_completion_queue_destroy(queues[thread_index]); diff --git a/test/core/surface/lame_client_test.c b/test/core/surface/lame_client_test.c index 79e53cb4222..c154d99d4cc 100644 --- a/test/core/surface/lame_client_test.c +++ b/test/core/surface/lame_client_test.c @@ -1,6 +1,6 @@ /* * - * Copyright 2015-2016, Google Inc. + * Copyright 2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without From a45782f090dfd997d2e80bb7761ddc8db4481b0b Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Fri, 11 Mar 2016 20:15:51 -0800 Subject: [PATCH 8/9] Fix copyright --- test/core/surface/concurrent_connectivity_test.c | 2 +- test/core/surface/lame_client_test.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/core/surface/concurrent_connectivity_test.c b/test/core/surface/concurrent_connectivity_test.c index 5361201db72..4d3b7bf22a3 100644 --- a/test/core/surface/concurrent_connectivity_test.c +++ b/test/core/surface/concurrent_connectivity_test.c @@ -1,6 +1,6 @@ /* * -* Copyright 2015-2016, Google Inc. +* Copyright 2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without diff --git a/test/core/surface/lame_client_test.c b/test/core/surface/lame_client_test.c index c154d99d4cc..79e53cb4222 100644 --- a/test/core/surface/lame_client_test.c +++ b/test/core/surface/lame_client_test.c @@ -1,6 +1,6 @@ /* * - * Copyright 2016, Google Inc. + * Copyright 2015-2016, Google Inc. * All rights reserved. * * Redistribution and use in source and binary forms, with or without From 822f1d7509fe7426a291de1054ea5e9f4892ee15 Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Sat, 12 Mar 2016 06:54:47 -0800 Subject: [PATCH 9/9] Add comment, tighten loop --- src/core/iomgr/timer.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/core/iomgr/timer.c b/src/core/iomgr/timer.c index 8badd83bbb1..f444643428d 100644 --- a/src/core/iomgr/timer.c +++ b/src/core/iomgr/timer.c @@ -33,11 +33,11 @@ #include "src/core/iomgr/timer.h" -#include "src/core/iomgr/timer_heap.h" -#include "src/core/iomgr/time_averaged_stats.h" #include #include #include +#include "src/core/iomgr/time_averaged_stats.h" +#include "src/core/iomgr/timer_heap.h" #define INVALID_HEAP_INDEX 0xffffffffu @@ -331,8 +331,17 @@ static int run_some_expired_timers(grpc_exec_ctx *exec_ctx, gpr_timespec now, gpr_mu_unlock(&g_mu); gpr_mu_unlock(&g_checker_mu); } else if (next != NULL) { + /* TODO(ctiller): this forces calling code to do an short poll, and + then retry the timer check (because this time through the timer list was + contended). + + We could reduce the cost here dramatically by keeping a count of how many + currently active pollers got through the uncontended case above + successfully, and waking up other pollers IFF that count drops to zero. + + Once that count is in place, this entire else branch could disappear. */ *next = gpr_time_min( - *next, gpr_time_add(now, gpr_time_from_millis(100, GPR_TIMESPAN))); + *next, gpr_time_add(now, gpr_time_from_millis(1, GPR_TIMESPAN))); } return (int)n;