Skip to content

RFC: non-blocking behavior for next_result #807

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 57 additions & 23 deletions ext/mysql2/client.c
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#include <mysql2_ext.h>
#include <violite.h>

#include <time.h>
#include <errno.h>
Expand Down Expand Up @@ -539,21 +540,35 @@ static VALUE disconnect_and_raise(VALUE self, VALUE error) {
return Qnil;
}

static VALUE do_query(void *args) {
struct async_query_args *async_args;
struct timeval tv;
struct timeval* tvp;
long int sec;
static void wait_for_fd(int fd, struct timeval *tvp) {
int retval;

for(;;) {
retval = rb_wait_for_single_fd(fd, RB_WAITFD_IN, tvp);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was surprised, so I double-checked that rb_wait_for_single_fd does allow a NULL timeval argument. Could you add a comment to let the future coders know that NULL can get through from get_read_timeout and is a legal value for this function?

https://github.com/ruby/ruby/search?utf8=%E2%9C%93&q=rb_wait_for_single_fd


if (retval == 0) {
rb_raise(cMysql2Error, "Timeout waiting for a response from the last query. (waited %d seconds)", FIX2INT(tvp->tv_sec));
}

if (retval < 0) {
rb_sys_fail(0);
}

if (retval > 0) {
break;
}
}
}

static struct timeval *get_read_timeout(VALUE self, struct timeval *tvp)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When this function returns it returns the pointer from the tvp argument?

{
long int sec;
VALUE read_timeout;

async_args = (struct async_query_args *)args;
read_timeout = rb_iv_get(async_args->self, "@read_timeout");
read_timeout = rb_iv_get(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 */
Expand All @@ -563,23 +578,20 @@ static VALUE do_query(void *args) {
rb_raise(cMysql2Error, "read_timeout must be a positive integer, you passed %ld", sec);
}
tvp->tv_usec = 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're in here, would you move the tv_usec = 0 to be directly below the tv_sec = sec line?

return tvp;
} else {
return NULL;
}
}

for(;;) {
retval = rb_wait_for_single_fd(async_args->fd, RB_WAITFD_IN, tvp);

if (retval == 0) {
rb_raise(cMysql2Error, "Timeout waiting for a response from the last query. (waited %d seconds)", FIX2INT(read_timeout));
}

if (retval < 0) {
rb_sys_fail(0);
}
static VALUE do_query(void *args) {
struct async_query_args *async_args;
struct timeval tv, *tvp;

if (retval > 0) {
break;
}
}
async_args = (struct async_query_args *)args;
tvp = get_read_timeout(async_args->self, &tv);
wait_for_fd(async_args->fd, tvp);

return Qnil;
}
Expand Down Expand Up @@ -1022,6 +1034,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
*
Expand All @@ -1031,8 +1049,24 @@ static VALUE rb_mysql_client_more_results(VALUE self)
static VALUE rb_mysql_client_next_result(VALUE self)
{
int ret;
struct timeval tv, *tvp;
GET_CLIENT(self);
ret = mysql_next_result(wrapper->client);

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.
*/
if ( !wrapper->client->net.vio->has_data(wrapper->client->net.vio) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is has_data the only method that required the violite.h headers?

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;
Expand Down
Loading