From 747946ec223bc96a6f1988b15589f9abee925a72 Mon Sep 17 00:00:00 2001 From: Ben Osheroff Date: Wed, 16 Nov 2016 10:33:00 -0800 Subject: [PATCH 1/5] change next_result to not block the interpreter when actually preforming a multi-statement select with any non-trivial components, the `next_result` call will block the ruby interpreter. We fix this in two ways: First, we wrap up the actual mysql_next_result call in a "no-gvl" call so that the interpreter can do other work while next_result is happening. Second, we borrow a method from do_query and spin in a select() loop on the mysql socket until the query is ready. A quick source dive into the underlying mysql source shows that next_result() falls into the same path as mysql_real_query (cli_read_query_result), so this should be a safe method. --- ext/mysql2/client.c | 49 +++++++++++++++++++++++++------------- spec/mysql2/client_spec.rb | 11 +++++++++ 2 files changed, 43 insertions(+), 17 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index d12f2c180..4190bfcf8 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -631,12 +631,31 @@ static VALUE disconnect_and_raise(VALUE self, VALUE error) { rb_exc_raise(error); } +static void wait_for_fd(int fd, struct timeval *tvp) { + int retval; + + for(;;) { + retval = rb_wait_for_single_fd(fd, RB_WAITFD_IN, tvp); + + if (retval == 0) { + rb_raise(cMysql2TimeoutError, "Timeout waiting for a response from the last query. (waited %ld seconds)", tvp->tv_sec); + } + + if (retval < 0) { + rb_sys_fail(0); + } + + if (retval > 0) { + break; + } + } +} + static VALUE do_query(void *args) { struct async_query_args *async_args = args; struct timeval tv; struct timeval *tvp; long int sec; - int retval; VALUE read_timeout; read_timeout = rb_iv_get(async_args->self, "@read_timeout"); @@ -656,21 +675,7 @@ static VALUE do_query(void *args) { tvp->tv_usec = 0; } - for(;;) { - retval = rb_wait_for_single_fd(async_args->fd, RB_WAITFD_IN, tvp); - - if (retval == 0) { - rb_raise(cMysql2TimeoutError, "Timeout waiting for a response from the last query. (waited %d seconds)", FIX2INT(read_timeout)); - } - - if (retval < 0) { - rb_sys_fail(0); - } - - if (retval > 0) { - break; - } - } + wait_for_fd(async_args->fd, tvp); return Qnil; } @@ -1129,6 +1134,12 @@ static VALUE rb_mysql_client_more_results(VALUE self) return Qtrue; } +static void *nogvl_next_result(void *ptr) { + mysql_client_wrapper *wrapper = ptr; + + return (void *)INT2NUM(mysql_next_result(wrapper->client)); +} + /* call-seq: * client.next_result * @@ -1139,7 +1150,11 @@ static VALUE rb_mysql_client_next_result(VALUE self) { int ret; GET_CLIENT(self); - ret = mysql_next_result(wrapper->client); + + wait_for_fd(wrapper->client->net.fd, NULL); + VALUE v = (VALUE)rb_thread_call_without_gvl(nogvl_next_result, wrapper, RUBY_UBF_IO, 0); + ret = NUM2INT(v); + if (ret > 0) { rb_raise_mysql2_error(wrapper); return Qfalse; diff --git a/spec/mysql2/client_spec.rb b/spec/mysql2/client_spec.rb index 00d9c17db..f9fe24eb7 100644 --- a/spec/mysql2/client_spec.rb +++ b/spec/mysql2/client_spec.rb @@ -774,6 +774,17 @@ def run_gc expect(@multi_client.more_results?).to be false end + it "should allow for interruption" do + time_top = Time.now.to_f + expect do + Timeout.timeout(0.2, ArgumentError) do + @multi_client.query('SELECT 1; SELECT SLEEP(2)') + @multi_client.next_result + end + end.to raise_error(ArgumentError) + expect(Time.now.to_f - time_top).to be <= 0.5 + end + it "#more_results? should work with stored procedures" do @multi_client.query("DROP PROCEDURE IF EXISTS test_proc") @multi_client.query("CREATE PROCEDURE test_proc() BEGIN SELECT 1 AS 'set_1'; SELECT 2 AS 'set_2'; END") From a5b1daa39c3345ab2ac08410e19e708ef8ddea55 Mon Sep 17 00:00:00 2001 From: Ben Osheroff Date: Sat, 19 Nov 2016 11:41:30 -0800 Subject: [PATCH 2/5] pick up violite.h to check for the state where mysql over-reads --- ext/mysql2/client.c | 83 ++++++++++++++++++++++++++++----------------- 1 file changed, 51 insertions(+), 32 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index 4190bfcf8..2417b36b3 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -1,4 +1,5 @@ #include +#include #include #include @@ -651,30 +652,35 @@ static void wait_for_fd(int fd, struct timeval *tvp) { } } -static VALUE do_query(void *args) { - struct async_query_args *async_args = args; - struct timeval tv; - struct timeval *tvp; +static struct timeval *get_read_timeout(VALUE self, struct timeval *tvp) { long int sec; VALUE read_timeout; - read_timeout = rb_iv_get(async_args->self, "@read_timeout"); - - tvp = NULL; - if (!NIL_P(read_timeout)) { - Check_Type(read_timeout, T_FIXNUM); - tvp = &tv; - sec = FIX2INT(read_timeout); - /* TODO: support partial seconds? - also, this check is here for sanity, we also check up in Ruby */ - if (sec >= 0) { - tvp->tv_sec = sec; - } else { - rb_raise(cMysql2Error, "read_timeout must be a positive integer, you passed %ld", sec); - } - tvp->tv_usec = 0; + read_timeout = rb_iv_get(self, "@read_timeout"); + + if (NIL_P(read_timeout)) { + return NULL; } + Check_Type(read_timeout, T_FIXNUM); + sec = FIX2INT(read_timeout); + /* TODO: support partial seconds? + also, this check is here for sanity, we also check up in Ruby */ + if (sec < 0) { + rb_raise(cMysql2Error, "read_timeout must be a positive integer, you passed %ld", sec); + } + + tvp->tv_sec = sec; + tvp->tv_usec = 0; + return tvp; +} + +static VALUE do_query(void *args) { + struct async_query_args *async_args; + struct timeval tv, *tvp; + + async_args = (struct async_query_args *)args; + tvp = get_read_timeout(async_args->self, &tv); wait_for_fd(async_args->fd, tvp); return Qnil; @@ -1146,23 +1152,36 @@ static void *nogvl_next_result(void *ptr) { * Fetch the next result set from the server. * Returns nothing. */ + static VALUE rb_mysql_client_next_result(VALUE self) { - int ret; - GET_CLIENT(self); + int ret; + struct timeval tv, *tvp; + GET_CLIENT(self); - wait_for_fd(wrapper->client->net.fd, NULL); - VALUE v = (VALUE)rb_thread_call_without_gvl(nogvl_next_result, wrapper, RUBY_UBF_IO, 0); - ret = NUM2INT(v); + if (mysql_more_results(wrapper->client) == 0) + return Qfalse; - if (ret > 0) { - rb_raise_mysql2_error(wrapper); - return Qfalse; - } else if (ret == 0) { - return Qtrue; - } else { - return Qfalse; - } + /* if both queries were ready by the time we finished the first query, + * the underlying mysql client library will have read both results into the buffer, + * defeating our attempt to poll() until ready. detect that case by peeking at the "vio" buffer. + */ + if ( !wrapper->client->net.vio->has_data(wrapper->client->net.vio) ) { + tvp = get_read_timeout(self, &tv); + wait_for_fd(wrapper->client->net.fd, tvp); + } + + VALUE v = (VALUE)rb_thread_call_without_gvl(nogvl_next_result, wrapper, RUBY_UBF_IO, 0); + ret = NUM2INT(v); + + if (ret > 0) { + rb_raise_mysql2_error(wrapper); + return Qfalse; + } else if (ret == 0) { + return Qtrue; + } else { + return Qfalse; + } } /* call-seq: From 2b11c2f32fcceaa0592acd0352c7c407d80e6ea1 Mon Sep 17 00:00:00 2001 From: Aaron Stone Date: Sat, 14 Apr 2018 11:42:51 -0700 Subject: [PATCH 3/5] Workaround to avoid including all of violite.h Rather than including violite.h to be able to look into the net.vio structure to call its has_data function pointer, we borrow the definitions of just two functions that understand this structure. The underlying client library may have pre-read the results from the next query, so wait_for_fd would not be triggered. Instead we will ask whether the net.vio structure has additional data that hasn't been parsed. Hack: use knowledge of whether the connecti is SSL or not to call the appropriate has_data function and pass net.vio as an opaque structure. Definitions are from vio_priv.h --- ext/mysql2/client.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index 2417b36b3..5da3b483a 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -1,5 +1,4 @@ #include -#include #include #include @@ -1146,13 +1145,21 @@ static void *nogvl_next_result(void *ptr) { return (void *)INT2NUM(mysql_next_result(wrapper->client)); } +/* Rather than including violite.h to be able to look into the net.vio structure + * to call its has_data function pointer, we borrow the definitions of just two + * functions that understand this structure. + * + * Definitions are from vio_priv.h + */ +my_bool vio_ssl_has_data (Vio *vio); +my_bool vio_buff_has_data (Vio *vio); + /* call-seq: * client.next_result * * Fetch the next result set from the server. * Returns nothing. */ - static VALUE rb_mysql_client_next_result(VALUE self) { int ret; @@ -1162,11 +1169,16 @@ static VALUE rb_mysql_client_next_result(VALUE self) if (mysql_more_results(wrapper->client) == 0) return Qfalse; - /* if both queries were ready by the time we finished the first query, - * the underlying mysql client library will have read both results into the buffer, - * defeating our attempt to poll() until ready. detect that case by peeking at the "vio" buffer. + /* The underlying client library may have pre-read the results from the next + * query, so wait_for_fd would not be triggered. Instead we will ask whether + * the net.vio structure has additional data that hasn't been parsed. + * + * Hack: use knowledge of whether the connecti is SSL or not to call the + * appropriate has_data function and pass net.vio as an opaque structure. */ - if ( !wrapper->client->net.vio->has_data(wrapper->client->net.vio) ) { + if ( mysql_get_ssl_cipher(wrapper->client) + ? !vio_ssl_has_data(wrapper->client->net.vio) + : !vio_buff_has_data(wrapper->client->net.vio) ) { tvp = get_read_timeout(self, &tv); wait_for_fd(wrapper->client->net.fd, tvp); } From 61d0900a4b48e7145314cd8aa18deac51ce84c27 Mon Sep 17 00:00:00 2001 From: Aaron Stone Date: Sat, 14 Apr 2018 11:14:57 -0700 Subject: [PATCH 4/5] Increase RuboCop maximum block size for the client spec again --- .rubocop_todo.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 3da4399a2..456fe2d28 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -22,7 +22,7 @@ Metrics/AbcSize: # Offense count: 31 # Configuration parameters: CountComments, ExcludedMethods. Metrics/BlockLength: - Max: 850 + Max: 855 # Offense count: 1 # Configuration parameters: CountBlocks. From 7a6dbdd1aaf54b4f035815f2dc97de893f7cf68e Mon Sep 17 00:00:00 2001 From: Aaron Stone Date: Sun, 15 Apr 2018 08:08:16 -0700 Subject: [PATCH 5/5] Use the existing macro for net.vio vs. net.pvio --- ext/mysql2/client.c | 34 ++++++++++++++++++++++------------ 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/ext/mysql2/client.c b/ext/mysql2/client.c index 5da3b483a..33ad9ace4 100644 --- a/ext/mysql2/client.c +++ b/ext/mysql2/client.c @@ -20,6 +20,15 @@ static VALUE sym_id, sym_version, sym_header_version, sym_async, sym_symbolize_k static VALUE sym_no_good_index_used, sym_no_index_used, sym_query_was_slow; static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args; +/* Rather than including violite.h to be able to look into the net.vio structure + * to call its has_data function pointer, we borrow the definitions of just two + * functions that understand this structure. + * + * Definitions are from vio_priv.h + */ +my_bool vio_ssl_has_data(void *); +my_bool vio_buff_has_data(void *); + #define REQUIRE_INITIALIZED(wrapper) \ if (!wrapper->initialized) { \ rb_raise(cMysql2Error, "MySQL client is not initialized"); \ @@ -27,8 +36,20 @@ static ID intern_brackets, intern_merge, intern_merge_bang, intern_new_with_args #if defined(HAVE_MYSQL_NET_VIO) || defined(HAVE_ST_NET_VIO) #define CONNECTED(wrapper) (wrapper->client->net.vio != NULL && wrapper->client->net.fd != -1) + + #define HAS_DATA(wrapper) \ + mysql_get_ssl_cipher(wrapper->client) \ + ? !vio_ssl_has_data(wrapper->client->net.vio) \ + : !vio_buff_has_data(wrapper->client->net.vio) + #elif defined(HAVE_MYSQL_NET_PVIO) || defined(HAVE_ST_NET_PVIO) #define CONNECTED(wrapper) (wrapper->client->net.pvio != NULL && wrapper->client->net.fd != -1) + + #define HAS_DATA(wrapper) \ + mysql_get_ssl_cipher(wrapper->client) \ + ? !pvio_ssl_has_data(wrapper->client->net.pvio) \ + : !pvio_buff_has_data(wrapper->client->net.pvio) + #endif #define REQUIRE_CONNECTED(wrapper) \ @@ -1145,15 +1166,6 @@ static void *nogvl_next_result(void *ptr) { return (void *)INT2NUM(mysql_next_result(wrapper->client)); } -/* Rather than including violite.h to be able to look into the net.vio structure - * to call its has_data function pointer, we borrow the definitions of just two - * functions that understand this structure. - * - * Definitions are from vio_priv.h - */ -my_bool vio_ssl_has_data (Vio *vio); -my_bool vio_buff_has_data (Vio *vio); - /* call-seq: * client.next_result * @@ -1176,9 +1188,7 @@ static VALUE rb_mysql_client_next_result(VALUE self) * Hack: use knowledge of whether the connecti is SSL or not to call the * appropriate has_data function and pass net.vio as an opaque structure. */ - if ( mysql_get_ssl_cipher(wrapper->client) - ? !vio_ssl_has_data(wrapper->client->net.vio) - : !vio_buff_has_data(wrapper->client->net.vio) ) { + if (HAS_DATA(wrapper)) { tvp = get_read_timeout(self, &tv); wait_for_fd(wrapper->client->net.fd, tvp); }