From b2c0b7bc7411c0914e2f65d56096ecde1a207b53 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Thu, 27 Apr 2017 00:26:25 -0700 Subject: constant state watch without timeouts --- src/ruby/end2end/channel_closing_driver.rb | 5 ++ src/ruby/end2end/channel_state_driver.rb | 3 + src/ruby/end2end/grpc_class_init_client.rb | 65 +++++++++++++++++----- src/ruby/end2end/grpc_class_init_driver.rb | 51 ++++++++++------- .../end2end/sig_int_during_channel_watch_client.rb | 2 + .../end2end/sig_int_during_channel_watch_driver.rb | 5 ++ 6 files changed, 96 insertions(+), 35 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/channel_closing_driver.rb b/src/ruby/end2end/channel_closing_driver.rb index d3e5373b0b..bed8c43405 100755 --- a/src/ruby/end2end/channel_closing_driver.rb +++ b/src/ruby/end2end/channel_closing_driver.rb @@ -61,6 +61,11 @@ def main 'channel is closed while connectivity is watched' end + client_exit_code = $CHILD_STATUS + if client_exit_code != 0 + fail "channel closing client failed, exit code #{client_exit_code}" + end + server_runner.stop end diff --git a/src/ruby/end2end/channel_state_driver.rb b/src/ruby/end2end/channel_state_driver.rb index 80fb62899e..9910076dba 100755 --- a/src/ruby/end2end/channel_state_driver.rb +++ b/src/ruby/end2end/channel_state_driver.rb @@ -58,6 +58,9 @@ def main 'It likely hangs when ended abruptly' end + # The interrupt in the child process should cause it to + # exit a non-zero status, so don't check it here. + # This test mainly tries to catch deadlock. server_runner.stop end diff --git a/src/ruby/end2end/grpc_class_init_client.rb b/src/ruby/end2end/grpc_class_init_client.rb index ee79292119..8e46907368 100755 --- a/src/ruby/end2end/grpc_class_init_client.rb +++ b/src/ruby/end2end/grpc_class_init_client.rb @@ -34,44 +34,81 @@ require_relative './end2end_common' -def main - grpc_class = '' - OptionParser.new do |opts| - opts.on('--grpc_class=P', String) do |p| - grpc_class = p +def construct_many(test_proc) + thds = [] + 4.times do + thds << Thread.new do + 20.times do + test_proc.call + end end - end.parse! + end + 20.times do + test_proc.call + end + thds.each(&:join) +end + +def run_gc_stress_test(test_proc) + GC.disable + construct_many(test_proc) - test_proc = nil + GC.enable + construct_many(test_proc) + GC.start(full_mark: true, immediate_sweep: true) + construct_many(test_proc) +end + +def get_test_proc(grpc_class) case grpc_class when 'channel' - test_proc = proc do + return proc do GRPC::Core::Channel.new('dummy_host', nil, :this_channel_is_insecure) end when 'server' - test_proc = proc do + return proc do GRPC::Core::Server.new({}) end when 'channel_credentials' - test_proc = proc do + return proc do GRPC::Core::ChannelCredentials.new end when 'call_credentials' - test_proc = proc do + return proc do GRPC::Core::CallCredentials.new(proc { |noop| noop }) end when 'compression_options' - test_proc = proc do + return proc do GRPC::Core::CompressionOptions.new end else fail "bad --grpc_class=#{grpc_class} param" end +end + +def main + grpc_class = '' + gc_stress = false + OptionParser.new do |opts| + opts.on('--grpc_class=P', String) do |p| + grpc_class = p + end + opts.on('--gc_stress=P') do |p| + gc_stress = p + end + end.parse! + + test_proc = get_test_proc(grpc_class) + + if gc_stress == 'true' + run_gc_stress_test(test_proc) + return + end - th = Thread.new { test_proc.call } + thd = Thread.new { test_proc.call } test_proc.call - th.join + thd.join end main diff --git a/src/ruby/end2end/grpc_class_init_driver.rb b/src/ruby/end2end/grpc_class_init_driver.rb index 764d029f14..0e330a493f 100755 --- a/src/ruby/end2end/grpc_class_init_driver.rb +++ b/src/ruby/end2end/grpc_class_init_driver.rb @@ -38,29 +38,38 @@ def main call_credentials compression_options ) - native_grpc_classes.each do |grpc_class| - STDERR.puts 'start client' - this_dir = File.expand_path(File.dirname(__FILE__)) - client_path = File.join(this_dir, 'grpc_class_init_client.rb') - client_pid = Process.spawn(RbConfig.ruby, - client_path, - "--grpc_class=#{grpc_class}") - begin - Timeout.timeout(10) do - Process.wait(client_pid) + # there is room for false positives in this test, + # do 10 runs for each config to reduce these. + [true, false].each do |gc_stress| + 10.times do + native_grpc_classes.each do |grpc_class| + STDERR.puts 'start client' + this_dir = File.expand_path(File.dirname(__FILE__)) + client_path = File.join(this_dir, 'grpc_class_init_client.rb') + client_pid = Process.spawn(RbConfig.ruby, + client_path, + "--grpc_class=#{grpc_class}", + "--gc_stress=#{gc_stress}") + begin + Timeout.timeout(10) do + Process.wait(client_pid) + end + rescue Timeout::Error + STDERR.puts "timeout waiting for client pid #{client_pid}" + Process.kill('SIGKILL', client_pid) + Process.wait(client_pid) + STDERR.puts 'killed client child' + raise 'Timed out waiting for client process. ' \ + 'It likely hangs when the first constructed gRPC object has ' \ + "type: #{grpc_class}" + end + + client_exit_code = $CHILD_STATUS + if client_exit_code != 0 + fail "client failed, exit code #{client_exit_code}" + end end - rescue Timeout::Error - STDERR.puts "timeout waiting for client pid #{client_pid}" - Process.kill('SIGKILL', client_pid) - Process.wait(client_pid) - STDERR.puts 'killed client child' - raise 'Timed out waiting for client process. ' \ - 'It likely hangs when the first constructed gRPC object has ' \ - "type: #{grpc_class}" end - - client_exit_code = $CHILD_STATUS - fail "client failed, exit code #{client_exit_code}" if client_exit_code != 0 end end diff --git a/src/ruby/end2end/sig_int_during_channel_watch_client.rb b/src/ruby/end2end/sig_int_during_channel_watch_client.rb index 389fc5ba33..0c6a374925 100755 --- a/src/ruby/end2end/sig_int_during_channel_watch_client.rb +++ b/src/ruby/end2end/sig_int_during_channel_watch_client.rb @@ -46,6 +46,8 @@ def main end end.parse! + trap('SIGINT') { exit 0 } + thd = Thread.new do child_thread_channel = GRPC::Core::Channel.new("localhost:#{server_port}", {}, diff --git a/src/ruby/end2end/sig_int_during_channel_watch_driver.rb b/src/ruby/end2end/sig_int_during_channel_watch_driver.rb index 670cda0919..79a8c133fa 100755 --- a/src/ruby/end2end/sig_int_during_channel_watch_driver.rb +++ b/src/ruby/end2end/sig_int_during_channel_watch_driver.rb @@ -63,6 +63,11 @@ def main 'SIGINT is sent while there is an active connectivity_state call' end + client_exit_code = $CHILD_STATUS + if client_exit_code != 0 + fail "sig_int_during_channel_watch_client failed: #{client_exit_code}" + end + server_runner.stop end -- cgit v1.2.3 From 7b3629e6c2570686701b4bdb6b171b219cbad06e Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 00:29:10 -0700 Subject: fix lack-of-abort bug --- src/ruby/end2end/grpc_class_init_client.rb | 16 +++++++++++++--- src/ruby/ext/grpc/rb_channel.c | 12 +----------- 2 files changed, 14 insertions(+), 14 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/grpc_class_init_client.rb b/src/ruby/end2end/grpc_class_init_client.rb index 8e46907368..62afc85b1d 100755 --- a/src/ruby/end2end/grpc_class_init_client.rb +++ b/src/ruby/end2end/grpc_class_init_client.rb @@ -106,9 +106,19 @@ def main return end - thd = Thread.new { test_proc.call } - test_proc.call - thd.join +# test_proc.call + + thds = [] + 100.times do + thds << Thread.new do + test_proc.call + sleep 10 + end + end + + #test_proc.call + raise "something" + thds.each(&:join) end main diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index c96dc672ce..748fe663ed 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -107,7 +107,6 @@ bg_watched_channel *bg_watched_channel_list_head = NULL; void grpc_rb_channel_try_register_connection_polling(bg_watched_channel *bg); void *wait_until_channel_polling_thread_started_no_gil(void*); -void wait_until_channel_polling_thread_started_unblocking_func(void*); void *channel_init_try_register_connection_polling_without_gil(void *arg); typedef struct channel_init_try_register_stack { @@ -228,7 +227,7 @@ VALUE grpc_rb_channel_init(int argc, VALUE *argv, VALUE self) { grpc_ruby_once_init(); rb_thread_call_without_gvl(wait_until_channel_polling_thread_started_no_gil, NULL, - wait_until_channel_polling_thread_started_unblocking_func, NULL); + run_poll_channels_loop_unblocking_func, NULL); /* "3" == 3 mandatory args */ rb_scan_args(argc, argv, "3", &target, &channel_args, &credentials); @@ -685,15 +684,6 @@ void *wait_until_channel_polling_thread_started_no_gil(void *arg) { return NULL; } -void wait_until_channel_polling_thread_started_unblocking_func(void* arg) { - (void)arg; - gpr_mu_lock(&global_connection_polling_mu); - gpr_log(GPR_DEBUG, "GRPC_RUBY: wait_until_channel_polling_thread_started_unblocking_func - begin aborting connection polling"); - abort_channel_polling = 1; - gpr_cv_broadcast(&global_connection_polling_cv); - gpr_mu_unlock(&global_connection_polling_mu); -} - static void *set_abort_channel_polling_without_gil(void *arg) { (void)arg; gpr_mu_lock(&global_connection_polling_mu); -- cgit v1.2.3 From fd4cbb70774a943a790459c8ec54d8bb112a3ef2 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 09:57:25 -0700 Subject: cleanup test --- src/ruby/end2end/grpc_class_init_client.rb | 57 ++++++++++++++++++++---------- src/ruby/end2end/grpc_class_init_driver.rb | 14 ++++---- 2 files changed, 46 insertions(+), 25 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/grpc_class_init_client.rb b/src/ruby/end2end/grpc_class_init_client.rb index 62afc85b1d..d9c23c3835 100755 --- a/src/ruby/end2end/grpc_class_init_client.rb +++ b/src/ruby/end2end/grpc_class_init_client.rb @@ -60,6 +60,27 @@ def run_gc_stress_test(test_proc) construct_many(test_proc) end +def run_concurrency_stress_test(test_proc) + test_proc.call + + thds = [] + 100.times do + thds << Thread.new do + test_proc.call + end + end + + raise "something" +end + +# default (no gc_stress and no concurrency_stress) +def run_default_test(test_proc) + thd = Thread.new do + test_proc.call + end + test_proc.call +end + def get_test_proc(grpc_class) case grpc_class when 'channel' @@ -89,36 +110,34 @@ end def main grpc_class = '' - gc_stress = false + stress_test = '' OptionParser.new do |opts| opts.on('--grpc_class=P', String) do |p| grpc_class = p end - opts.on('--gc_stress=P') do |p| - gc_stress = p + opts.on('--stress_test=P') do |p| + stress_test = p end end.parse! test_proc = get_test_proc(grpc_class) - if gc_stress == 'true' + # the different test configs need to be ran + # in separate processes, since each one tests + # clean shutdown in a different way + case stress_test + when 'gc' + p 'run gc stress' run_gc_stress_test(test_proc) - return - end - -# test_proc.call - - thds = [] - 100.times do - thds << Thread.new do - test_proc.call - sleep 10 - end + when 'concurrency' + p 'run concurrency stress' + run_concurrency_stress_test(test_proc) + when '' + p 'run default' + run_default_test(test_proc) + else + fail "bad --stress_test=#{stress_test} param" end - - #test_proc.call - raise "something" - thds.each(&:join) end main diff --git a/src/ruby/end2end/grpc_class_init_driver.rb b/src/ruby/end2end/grpc_class_init_driver.rb index 0e330a493f..195da3cf9f 100755 --- a/src/ruby/end2end/grpc_class_init_driver.rb +++ b/src/ruby/end2end/grpc_class_init_driver.rb @@ -39,17 +39,17 @@ def main compression_options ) # there is room for false positives in this test, - # do 10 runs for each config to reduce these. - [true, false].each do |gc_stress| - 10.times do - native_grpc_classes.each do |grpc_class| + # do a few runs for each config + 4.times do + native_grpc_classes.each do |grpc_class| + ['', 'gc', 'concurrency'].each do |stress_test_type| STDERR.puts 'start client' this_dir = File.expand_path(File.dirname(__FILE__)) client_path = File.join(this_dir, 'grpc_class_init_client.rb') client_pid = Process.spawn(RbConfig.ruby, client_path, "--grpc_class=#{grpc_class}", - "--gc_stress=#{gc_stress}") + "--stress_test=#{stress_test_type}") begin Timeout.timeout(10) do Process.wait(client_pid) @@ -65,7 +65,9 @@ def main end client_exit_code = $CHILD_STATUS - if client_exit_code != 0 + # concurrency stress test type is expected to exit with a + # non-zero status due to an exception being raised + if client_exit_code != 0 and stress_test_type != 'concurrency' fail "client failed, exit code #{client_exit_code}" end end -- cgit v1.2.3 From 916893b618d602fef2ec1dbf9821611f67a7a1d3 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 10:05:39 -0700 Subject: fix up the test --- src/ruby/end2end/grpc_class_init_client.rb | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/grpc_class_init_client.rb b/src/ruby/end2end/grpc_class_init_client.rb index d9c23c3835..464c42d07a 100755 --- a/src/ruby/end2end/grpc_class_init_client.rb +++ b/src/ruby/end2end/grpc_class_init_client.rb @@ -61,16 +61,15 @@ def run_gc_stress_test(test_proc) end def run_concurrency_stress_test(test_proc) - test_proc.call - - thds = [] 100.times do - thds << Thread.new do + Thread.new do test_proc.call end end - raise "something" + test_proc.call + + raise 'exception thrown while child thread initing class' end # default (no gc_stress and no concurrency_stress) -- cgit v1.2.3 From 5c6dda8639bd390565e794192ddfb15af0837c92 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 13:08:34 -0700 Subject: fix tentative startup bug --- src/ruby/end2end/grpc_class_init_client.rb | 3 ++- src/ruby/end2end/grpc_class_init_driver.rb | 6 +++--- src/ruby/ext/grpc/rb_channel.c | 5 ++--- src/ruby/ext/grpc/rb_grpc.c | 21 +++++++++++++++++++-- 4 files changed, 26 insertions(+), 9 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/grpc_class_init_client.rb b/src/ruby/end2end/grpc_class_init_client.rb index 464c42d07a..e73ca76850 100755 --- a/src/ruby/end2end/grpc_class_init_client.rb +++ b/src/ruby/end2end/grpc_class_init_client.rb @@ -69,7 +69,7 @@ def run_concurrency_stress_test(test_proc) test_proc.call - raise 'exception thrown while child thread initing class' + fail 'exception thrown while child thread initing class' end # default (no gc_stress and no concurrency_stress) @@ -78,6 +78,7 @@ def run_default_test(test_proc) test_proc.call end test_proc.call + thd.join end def get_test_proc(grpc_class) diff --git a/src/ruby/end2end/grpc_class_init_driver.rb b/src/ruby/end2end/grpc_class_init_driver.rb index 195da3cf9f..c65ed547c5 100755 --- a/src/ruby/end2end/grpc_class_init_driver.rb +++ b/src/ruby/end2end/grpc_class_init_driver.rb @@ -65,9 +65,9 @@ def main end client_exit_code = $CHILD_STATUS - # concurrency stress test type is expected to exit with a - # non-zero status due to an exception being raised - if client_exit_code != 0 and stress_test_type != 'concurrency' + # concurrency stress test type is expected to exit with a + # non-zero status due to an exception being raised + if client_exit_code != 0 && stress_test_type != 'concurrency' fail "client failed, exit code #{client_exit_code}" end end diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index 748fe663ed..4e59174c3b 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -368,8 +368,8 @@ void *wait_for_watch_state_op_complete_without_gvl(void *arg) { * state changes from "last_state". * */ VALUE grpc_rb_channel_watch_connectivity_state(VALUE self, - VALUE last_state, - VALUE deadline) { + VALUE last_state, + VALUE deadline) { grpc_rb_channel *wrapper = NULL; watch_state_stack stack; void* op_success = 0; @@ -693,7 +693,6 @@ static void *set_abort_channel_polling_without_gil(void *arg) { return NULL; } - /* Temporary fix for * https://github.com/GoogleCloudPlatform/google-cloud-ruby/issues/899. * Transports in idle channels can get destroyed. Normally c-core re-connects, diff --git a/src/ruby/ext/grpc/rb_grpc.c b/src/ruby/ext/grpc/rb_grpc.c index 584b5dbc63..2a3b339102 100644 --- a/src/ruby/ext/grpc/rb_grpc.c +++ b/src/ruby/ext/grpc/rb_grpc.c @@ -42,6 +42,7 @@ #include #include +#include #include "rb_call.h" #include "rb_call_credentials.h" #include "rb_channel.h" @@ -295,11 +296,12 @@ static gpr_once g_once_init = GPR_ONCE_INIT; static void grpc_ruby_once_init_internal() { grpc_init(); - grpc_rb_event_queue_thread_start(); - grpc_rb_channel_polling_thread_start(); atexit(grpc_rb_shutdown); } +static VALUE bg_thread_init_rb_mu = Qundef; +static int bg_thread_init_done = 0; + void grpc_ruby_once_init() { /* ruby_vm_at_exit doesn't seem to be working. It would crash once every * blue moon, and some users are getting it repeatedly. See the discussions @@ -312,6 +314,18 @@ void grpc_ruby_once_init() { * schedule our initialization and destruction only once. */ gpr_once_init(&g_once_init, grpc_ruby_once_init_internal); + + // Avoid calling calling into ruby library (when creating threads here) + // in gpr_once_init. In general, it appears to be unsafe to call + // into the ruby library while holding a non-ruby mutex, because a gil yield + // could end up trying to lock onto that same mutex and deadlocking. + rb_mutex_lock(bg_thread_init_rb_mu); + if (!bg_thread_init_done) { + grpc_rb_event_queue_thread_start(); + grpc_rb_channel_polling_thread_start(); + bg_thread_init_done = 1; + } + rb_mutex_unlock(bg_thread_init_rb_mu); } void Init_grpc_c() { @@ -320,6 +334,9 @@ void Init_grpc_c() { return; } + bg_thread_init_rb_mu = rb_mutex_new(); + rb_global_variable(&bg_thread_init_rb_mu); + grpc_rb_mGRPC = rb_define_module("GRPC"); grpc_rb_mGrpcCore = rb_define_module_under(grpc_rb_mGRPC, "Core"); grpc_rb_sNewServerRpc = -- cgit v1.2.3 From c24d53b0cfffe2cd78d53c0c86de43346dcc7ee7 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 20:25:38 -0700 Subject: api watch unblock func kills only its own channel --- .../multiple_killed_watching_threads_driver.rb | 56 ++++++++++++++++++++++ src/ruby/ext/grpc/rb_channel.c | 18 ++++--- .../helper_scripts/run_ruby_end2end_tests.sh | 1 + 3 files changed, 69 insertions(+), 6 deletions(-) create mode 100755 src/ruby/end2end/multiple_killed_watching_threads_driver.rb (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb new file mode 100755 index 0000000000..0c98915b7e --- /dev/null +++ b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb @@ -0,0 +1,56 @@ +#!/usr/bin/env ruby + +# 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. + +require_relative './end2end_common' + +Thread.abort_on_exception = true + +include GRPC::Core::ConnectivityStates + +def watch_state(ch) + thd = Thread.new do + state = ch.connectivity_state(false) + fail "non-idle state: #{state}" unless state == IDLE + ch.watch_connectivity_state(IDLE, Time.now + 360) + end + sleep 0.1 + thd.kill +end + +def main + 10.times do + ch = GRPC::Core::Channel.new('dummy_host', + nil, :this_channel_is_insecure) + watch_state(ch) + end +end + +main diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index e02dd0805d..6e7baa3122 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -366,6 +366,16 @@ static void *wait_for_watch_state_op_complete_without_gvl(void *arg) { return success; } +static void wait_for_watch_state_op_complete_unblocking_func(void *arg) { + bg_watched_channel *bg = (bg_watched_channel *)arg; + gpr_mu_lock(&global_connection_polling_mu); + if (!bg->channel_destroyed) { + grpc_channel_destroy(bg->channel); + bg->channel_destroyed = 1; + } + gpr_mu_unlock(&global_connection_polling_mu); +} + /* Wait until the channel's connectivity state becomes different from * "last_state", or "deadline" expires. * Returns true if the the channel's connectivity state becomes @@ -400,7 +410,7 @@ static VALUE grpc_rb_channel_watch_connectivity_state(VALUE self, op_success = rb_thread_call_without_gvl( wait_for_watch_state_op_complete_without_gvl, &stack, - run_poll_channels_loop_unblocking_func, NULL); + wait_for_watch_state_op_complete_unblocking_func, wrapper->bg_wrapped); return op_success ? Qtrue : Qfalse; } @@ -577,11 +587,7 @@ static void grpc_rb_channel_try_register_connection_polling( return; } GPR_ASSERT(bg->refcount == 1); - if (bg->channel_destroyed) { - GPR_ASSERT(abort_channel_polling); - return; - } - if (abort_channel_polling) { + if (bg->channel_destroyed || abort_channel_polling) { return; } diff --git a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh index 6688025260..ab882d62bc 100755 --- a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh +++ b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh @@ -41,4 +41,5 @@ ruby src/ruby/end2end/sig_int_during_channel_watch_driver.rb || EXIT_CODE=1 ruby src/ruby/end2end/killed_client_thread_driver.rb || EXIT_CODE=1 ruby src/ruby/end2end/forking_client_driver.rb || EXIT_CODE=1 ruby src/ruby/end2end/grpc_class_init_driver.rb || EXIT_CODE=1 +ruby src/ruby/end2end/multiple_killed_watching_threads_driver.rb || EXIT_CODE=1 exit $EXIT_CODE -- cgit v1.2.3 From d7455abfbd1394a6984b901a305785e9cba7b2d6 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Wed, 17 May 2017 21:39:09 -0700 Subject: make get conn state always safe to call --- src/ruby/end2end/multiple_killed_watching_threads_driver.rb | 7 +++++++ src/ruby/ext/grpc/rb_channel.c | 12 ++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) (limited to 'src/ruby/end2end') diff --git a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb index 0c98915b7e..206ec8e801 100755 --- a/src/ruby/end2end/multiple_killed_watching_threads_driver.rb +++ b/src/ruby/end2end/multiple_killed_watching_threads_driver.rb @@ -46,10 +46,17 @@ def watch_state(ch) end def main + channels = [] 10.times do ch = GRPC::Core::Channel.new('dummy_host', nil, :this_channel_is_insecure) watch_state(ch) + channels << ch + end + + # checking state should still be safe to call + channels.each do |c| + fail unless c.connectivity_state(false) == FATAL_FAILURE end end diff --git a/src/ruby/ext/grpc/rb_channel.c b/src/ruby/ext/grpc/rb_channel.c index 6e7baa3122..0524c49710 100644 --- a/src/ruby/ext/grpc/rb_channel.c +++ b/src/ruby/ext/grpc/rb_channel.c @@ -273,7 +273,7 @@ static VALUE grpc_rb_channel_init(int argc, VALUE *argv, VALUE self) { } typedef struct get_state_stack { - grpc_channel *channel; + bg_watched_channel *bg; int try_to_connect; int out; } get_state_stack; @@ -283,14 +283,10 @@ static void *get_state_without_gil(void *arg) { gpr_mu_lock(&global_connection_polling_mu); GPR_ASSERT(abort_channel_polling || channel_polling_thread_started); - if (abort_channel_polling) { - // Assume that this channel has been destroyed by the - // background thread. - // The case in which the channel polling thread - // failed to start just always shows shutdown state. + if (stack->bg->channel_destroyed) { stack->out = GRPC_CHANNEL_SHUTDOWN; } else { - stack->out = grpc_channel_check_connectivity_state(stack->channel, + stack->out = grpc_channel_check_connectivity_state(stack->bg->channel, stack->try_to_connect); } gpr_mu_unlock(&global_connection_polling_mu); @@ -322,7 +318,7 @@ static VALUE grpc_rb_channel_get_connectivity_state(int argc, VALUE *argv, return Qnil; } - stack.channel = wrapper->bg_wrapped->channel; + stack.bg = wrapper->bg_wrapped; stack.try_to_connect = RTEST(try_to_connect_param) ? 1 : 0; rb_thread_call_without_gvl(get_state_without_gil, &stack, NULL, NULL); -- cgit v1.2.3