From 27338de44531467941f5dafb823560c0e1e3d401 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Thu, 23 Mar 2017 18:23:49 -0700 Subject: [PATCH 1/5] add test in that sends a sigint to client while its making an rpc ona child thread - segfaults on mac --- .../end2end/killed_client_thread_client.rb | 55 ++++++++ .../end2end/killed_client_thread_driver.rb | 132 ++++++++++++++++++ 2 files changed, 187 insertions(+) create mode 100755 src/ruby/end2end/killed_client_thread_client.rb create mode 100755 src/ruby/end2end/killed_client_thread_driver.rb diff --git a/src/ruby/end2end/killed_client_thread_client.rb b/src/ruby/end2end/killed_client_thread_client.rb new file mode 100755 index 00000000000..9a69161736e --- /dev/null +++ b/src/ruby/end2end/killed_client_thread_client.rb @@ -0,0 +1,55 @@ +#!/usr/bin/env ruby + +# Copyright 2015, 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' + +def main + server_port = '' + OptionParser.new do |opts| + opts.on('--client_control_port=P', String) do + STDERR.puts 'client control port not used' + end + opts.on('--server_port=P', String) do |p| + server_port = p + end + end.parse! + + thd = Thread.new do + stub = Echo::EchoServer::Stub.new("localhost:#{server_port}", + :this_channel_is_insecure) + stub.echo(Echo::EchoRequest.new(request: 'hello')) + raise 'the clients rpc in this test shouldnt complete. ' \ + 'expecting SIGINT to happen in the middle of the call' + end + thd.join +end + +main diff --git a/src/ruby/end2end/killed_client_thread_driver.rb b/src/ruby/end2end/killed_client_thread_driver.rb new file mode 100755 index 00000000000..71d5db276df --- /dev/null +++ b/src/ruby/end2end/killed_client_thread_driver.rb @@ -0,0 +1,132 @@ +#!/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' + +class SleepingEchoServerImpl < Echo::EchoServer::Service + def initialize(call_started, call_started_mu, call_started_cv) + @call_started = call_started + @call_started_mu = call_started_mu + @call_started_cv = call_started_cv + end + def echo(echo_req, _) + @call_started_mu.synchronize do + @call_started.set_true + @call_started_cv.signal + end + sleep 1000 + Echo::EchoReply.new(response: echo_req.request) + end +end + +class SleepingServerRunner + def initialize(service_impl) + @service_impl = service_impl + end + + def run + @srv = GRPC::RpcServer.new + port = @srv.add_http2_port('0.0.0.0:0', :this_port_is_insecure) + @srv.handle(@service_impl) + + @thd = Thread.new do + @srv.run + end + @srv.wait_till_running + port + end + + def stop + @srv.stop + @thd.join + fail 'server not stopped' unless @srv.stopped? + end +end + +class BoolHolder + def init + @val = false + end + def set_true + @val = true + end + def get_val + @val + end +end + + +def main + STDERR.puts 'start server' + + call_started = BoolHolder.new + call_started_mu = Mutex.new + call_started_cv = ConditionVariable.new + + service_impl = SleepingEchoServerImpl.new(call_started, call_started_mu, call_started_cv) + server_runner = SleepingServerRunner.new(service_impl) + server_port = server_runner.run + + STDERR.puts 'start client' + _, client_pid = start_client('killed_client_thread_client.rb', + server_port) + + call_started_mu.synchronize do + while !call_started.get_val do + call_started_cv.wait(call_started_mu) + end + end + + Process.kill('SIGINT', client_pid) + STDERR.puts 'sent shutdown' + + begin + Timeout.timeout(10) do + Process.wait(client_pid) + end + rescue Timeout::Error + STDERR.puts "timeout wait 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 ' \ + 'killed while in the middle of an rpc' + end + + client_exit_code = $CHILD_STATUS + if client_exit_code.termsig != 2 # SIGINT + fail "expected client exit from SIGINT but got child status: #{client_exit_code}" + end + + server_runner.stop +end + +main From 34bb6df108742cf5c17d019ee5d010c7ff12a55b Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Thu, 23 Mar 2017 22:18:02 -0700 Subject: [PATCH 2/5] allocated run batch stack on the heap --- src/ruby/ext/grpc/rb_call.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c index 82d340b2544..cf62564667a 100644 --- a/src/ruby/ext/grpc/rb_call.c +++ b/src/ruby/ext/grpc/rb_call.c @@ -784,7 +784,7 @@ static VALUE grpc_run_batch_stack_build_result(run_batch_stack *st) { Only one operation of each type can be active at once in any given batch */ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { - run_batch_stack st; + run_batch_stack *st; grpc_rb_call *call = NULL; grpc_event ev; grpc_call_error err; @@ -792,6 +792,8 @@ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { VALUE rb_write_flag = rb_ivar_get(self, id_write_flag); unsigned write_flag = 0; void *tag = (void*)&st; + st = gpr_malloc(sizeof(run_batch_stack)); + if (RTYPEDDATA_DATA(self) == NULL) { rb_raise(grpc_rb_eCallError, "Cannot run batch on closed call"); return Qnil; @@ -806,14 +808,15 @@ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { if (rb_write_flag != Qnil) { write_flag = NUM2UINT(rb_write_flag); } - grpc_run_batch_stack_init(&st, write_flag); - grpc_run_batch_stack_fill_ops(&st, ops_hash); + grpc_run_batch_stack_init(st, write_flag); + grpc_run_batch_stack_fill_ops(st, ops_hash); /* call grpc_call_start_batch, then wait for it to complete using * pluck_event */ - err = grpc_call_start_batch(call->wrapped, st.ops, st.op_num, tag, NULL); + err = grpc_call_start_batch(call->wrapped, st->ops, st->op_num, tag, NULL); if (err != GRPC_CALL_OK) { - grpc_run_batch_stack_cleanup(&st); + grpc_run_batch_stack_cleanup(st); + gpr_free(st); rb_raise(grpc_rb_eCallError, "grpc_call_start_batch failed with %s (code=%d)", grpc_call_error_detail_of(err), err); @@ -826,8 +829,9 @@ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { } /* Build and return the BatchResult struct result, if there is an error, it's reflected in the status */ - result = grpc_run_batch_stack_build_result(&st); - grpc_run_batch_stack_cleanup(&st); + result = grpc_run_batch_stack_build_result(st); + grpc_run_batch_stack_cleanup(st); + gpr_free(st); return result; } From 077f890965766b13d3a5d19a1589b987e47c0570 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Fri, 24 Mar 2017 09:53:40 -0700 Subject: [PATCH 3/5] conform test to formatter --- src/ruby/end2end/channel_closing_driver.rb | 2 +- src/ruby/end2end/channel_state_driver.rb | 2 +- src/ruby/end2end/end2end_common.rb | 5 +- .../end2end/killed_client_thread_client.rb | 5 +- .../end2end/killed_client_thread_driver.rb | 56 +++++++------------ src/ruby/end2end/sig_handling_driver.rb | 2 +- .../sig_int_during_channel_watch_driver.rb | 2 +- .../helper_scripts/run_ruby_end2end_tests.sh | 1 + 8 files changed, 31 insertions(+), 44 deletions(-) diff --git a/src/ruby/end2end/channel_closing_driver.rb b/src/ruby/end2end/channel_closing_driver.rb index 43e2fe8cbbd..d3e5373b0bb 100755 --- a/src/ruby/end2end/channel_closing_driver.rb +++ b/src/ruby/end2end/channel_closing_driver.rb @@ -36,7 +36,7 @@ require_relative './end2end_common' def main STDERR.puts 'start server' - server_runner = ServerRunner.new + server_runner = ServerRunner.new(EchoServerImpl) server_port = server_runner.run sleep 1 diff --git a/src/ruby/end2end/channel_state_driver.rb b/src/ruby/end2end/channel_state_driver.rb index c3184bf9392..80fb62899e5 100755 --- a/src/ruby/end2end/channel_state_driver.rb +++ b/src/ruby/end2end/channel_state_driver.rb @@ -35,7 +35,7 @@ require_relative './end2end_common' def main STDERR.puts 'start server' - server_runner = ServerRunner.new + server_runner = ServerRunner.new(EchoServerImpl) server_port = server_runner.run sleep 1 diff --git a/src/ruby/end2end/end2end_common.rb b/src/ruby/end2end/end2end_common.rb index 9534bb20787..1c87ceddf1d 100755 --- a/src/ruby/end2end/end2end_common.rb +++ b/src/ruby/end2end/end2end_common.rb @@ -55,13 +55,14 @@ end # ServerRunner starts an "echo server" that test clients can make calls to class ServerRunner - def initialize + def initialize(service_impl) + @service_impl = service_impl end def run @srv = GRPC::RpcServer.new port = @srv.add_http2_port('0.0.0.0:0', :this_port_is_insecure) - @srv.handle(EchoServerImpl) + @srv.handle(@service_impl) @thd = Thread.new do @srv.run diff --git a/src/ruby/end2end/killed_client_thread_client.rb b/src/ruby/end2end/killed_client_thread_client.rb index 9a69161736e..d5a7db7d586 100755 --- a/src/ruby/end2end/killed_client_thread_client.rb +++ b/src/ruby/end2end/killed_client_thread_client.rb @@ -29,6 +29,9 @@ # (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE # OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# Attempt to reproduce +# https://github.com/GoogleCloudPlatform/google-cloud-ruby/issues/1327 + require_relative './end2end_common' def main @@ -46,7 +49,7 @@ def main stub = Echo::EchoServer::Stub.new("localhost:#{server_port}", :this_channel_is_insecure) stub.echo(Echo::EchoRequest.new(request: 'hello')) - raise 'the clients rpc in this test shouldnt complete. ' \ + fail 'the clients rpc in this test shouldnt complete. ' \ 'expecting SIGINT to happen in the middle of the call' end thd.join diff --git a/src/ruby/end2end/killed_client_thread_driver.rb b/src/ruby/end2end/killed_client_thread_driver.rb index 71d5db276df..2963a287c59 100755 --- a/src/ruby/end2end/killed_client_thread_driver.rb +++ b/src/ruby/end2end/killed_client_thread_driver.rb @@ -31,12 +31,15 @@ require_relative './end2end_common' +# Service that sleeps for a long time upon receiving an 'echo request' +# Also, this notified @call_started_cv once it has received a request. class SleepingEchoServerImpl < Echo::EchoServer::Service def initialize(call_started, call_started_mu, call_started_cv) @call_started = call_started @call_started_mu = call_started_mu @call_started_cv = call_started_cv end + def echo(echo_req, _) @call_started_mu.synchronize do @call_started.set_true @@ -47,43 +50,19 @@ class SleepingEchoServerImpl < Echo::EchoServer::Service end end -class SleepingServerRunner - def initialize(service_impl) - @service_impl = service_impl - end - - def run - @srv = GRPC::RpcServer.new - port = @srv.add_http2_port('0.0.0.0:0', :this_port_is_insecure) - @srv.handle(@service_impl) - - @thd = Thread.new do - @srv.run - end - @srv.wait_till_running - port - end - - def stop - @srv.stop - @thd.join - fail 'server not stopped' unless @srv.stopped? - end -end - +# Mutable boolean class BoolHolder + attr_reader :val + def init @val = false end + def set_true @val = true end - def get_val - @val - end end - def main STDERR.puts 'start server' @@ -91,20 +70,22 @@ def main call_started_mu = Mutex.new call_started_cv = ConditionVariable.new - service_impl = SleepingEchoServerImpl.new(call_started, call_started_mu, call_started_cv) - server_runner = SleepingServerRunner.new(service_impl) + service_impl = SleepingEchoServerImpl.new(call_started, + call_started_mu, + call_started_cv) + server_runner = ServerRunner.new(service_impl) server_port = server_runner.run STDERR.puts 'start client' _, client_pid = start_client('killed_client_thread_client.rb', - server_port) + server_port) call_started_mu.synchronize do - while !call_started.get_val do - call_started_cv.wait(call_started_mu) - end + call_started_cv.wait(call_started_mu) until call_started.val end + # SIGINT the child process not that it's + # in the middle of an RPC (happening on a non-main thread) Process.kill('SIGINT', client_pid) STDERR.puts 'sent shutdown' @@ -117,13 +98,14 @@ def main 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 ' \ - 'killed while in the middle of an rpc' + raise 'Timed out waiting for client process. ' \ + 'It likely hangs when killed while in the middle of an rpc' end client_exit_code = $CHILD_STATUS if client_exit_code.termsig != 2 # SIGINT - fail "expected client exit from SIGINT but got child status: #{client_exit_code}" + fail 'expected client exit from SIGINT ' \ + "but got child status: #{client_exit_code}" end server_runner.stop diff --git a/src/ruby/end2end/sig_handling_driver.rb b/src/ruby/end2end/sig_handling_driver.rb index c5d46e074cb..6691464dc69 100755 --- a/src/ruby/end2end/sig_handling_driver.rb +++ b/src/ruby/end2end/sig_handling_driver.rb @@ -36,7 +36,7 @@ require_relative './end2end_common' def main STDERR.puts 'start server' - server_runner = ServerRunner.new + server_runner = ServerRunner.new(EchoServerImpl) server_port = server_runner.run sleep 1 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 84d039bf19d..670cda0919f 100755 --- a/src/ruby/end2end/sig_int_during_channel_watch_driver.rb +++ b/src/ruby/end2end/sig_int_during_channel_watch_driver.rb @@ -36,7 +36,7 @@ require_relative './end2end_common' def main STDERR.puts 'start server' - server_runner = ServerRunner.new + server_runner = ServerRunner.new(EchoServerImpl) server_port = server_runner.run sleep 1 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 d7da6364d8f..92d69757073 100755 --- a/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh +++ b/tools/run_tests/helper_scripts/run_ruby_end2end_tests.sh @@ -38,4 +38,5 @@ ruby src/ruby/end2end/sig_handling_driver.rb || EXIT_CODE=1 ruby src/ruby/end2end/channel_state_driver.rb || EXIT_CODE=1 ruby src/ruby/end2end/channel_closing_driver.rb || EXIT_CODE=1 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 exit $EXIT_CODE From 8d8dce8db31dc9f54f606417ed6bc1ec93f9d76b Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Fri, 24 Mar 2017 10:32:15 -0700 Subject: [PATCH 4/5] malloc run_batch_stack after type checks --- src/ruby/ext/grpc/rb_call.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ruby/ext/grpc/rb_call.c b/src/ruby/ext/grpc/rb_call.c index cf62564667a..344cb941ffb 100644 --- a/src/ruby/ext/grpc/rb_call.c +++ b/src/ruby/ext/grpc/rb_call.c @@ -784,7 +784,7 @@ static VALUE grpc_run_batch_stack_build_result(run_batch_stack *st) { Only one operation of each type can be active at once in any given batch */ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { - run_batch_stack *st; + run_batch_stack *st = NULL; grpc_rb_call *call = NULL; grpc_event ev; grpc_call_error err; @@ -792,7 +792,6 @@ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { VALUE rb_write_flag = rb_ivar_get(self, id_write_flag); unsigned write_flag = 0; void *tag = (void*)&st; - st = gpr_malloc(sizeof(run_batch_stack)); if (RTYPEDDATA_DATA(self) == NULL) { rb_raise(grpc_rb_eCallError, "Cannot run batch on closed call"); @@ -808,6 +807,7 @@ static VALUE grpc_rb_call_run_batch(VALUE self, VALUE ops_hash) { if (rb_write_flag != Qnil) { write_flag = NUM2UINT(rb_write_flag); } + st = gpr_malloc(sizeof(run_batch_stack)); grpc_run_batch_stack_init(st, write_flag); grpc_run_batch_stack_fill_ops(st, ops_hash); From 4364ded9b16c5a0e1259af7423780c254d0465e6 Mon Sep 17 00:00:00 2001 From: Alexander Polcyn Date: Fri, 24 Mar 2017 10:41:37 -0700 Subject: [PATCH 5/5] wording fix in comments --- src/ruby/end2end/killed_client_thread_driver.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ruby/end2end/killed_client_thread_driver.rb b/src/ruby/end2end/killed_client_thread_driver.rb index 2963a287c59..f76d3e1746d 100755 --- a/src/ruby/end2end/killed_client_thread_driver.rb +++ b/src/ruby/end2end/killed_client_thread_driver.rb @@ -32,7 +32,7 @@ require_relative './end2end_common' # Service that sleeps for a long time upon receiving an 'echo request' -# Also, this notified @call_started_cv once it has received a request. +# Also, this notifies @call_started_cv once it has received a request. class SleepingEchoServerImpl < Echo::EchoServer::Service def initialize(call_started, call_started_mu, call_started_cv) @call_started = call_started @@ -84,7 +84,7 @@ def main call_started_cv.wait(call_started_mu) until call_started.val end - # SIGINT the child process not that it's + # SIGINT the child process now that it's # in the middle of an RPC (happening on a non-main thread) Process.kill('SIGINT', client_pid) STDERR.puts 'sent shutdown'