Skip to content
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

Handle read_timeout for queries executed within EventMachine #971

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle read_timeout for queries executed within EventMachine #971

wants to merge 2 commits into from

Conversation

gmodarelli
Copy link

Hello, I'm not sure if this is the best approach to handle a read_timeout when a query is being run within an EventMachine reactor loop, but I wanted to give it a try and pick your brain on the topic :)

The read_timeout option is only used when a query is executed synchronously, so it will be completely ignored for EM clients.

mysql2/ext/mysql2/client.c

Lines 782 to 792 in b3fe727

rb_rescue2(do_send_query, (VALUE)&args, disconnect_and_raise, self, rb_eException, (VALUE)0);
if (rb_hash_aref(current, sym_async) == Qtrue) {
return Qnil;
} else {
async_args.fd = wrapper->client->net.fd;
async_args.self = self;
rb_rescue2(do_query, (VALUE)&async_args, disconnect_and_raise, self, rb_eException, (VALUE)0);
return rb_ensure(rb_mysql_client_async_result, self, disconnect_and_mark_inactive, self);

In order to expose a similar behaviour I've introduced a Deferrable Timeout which will raise a Mysql2::EM::ReadTimeout.

In case of a read timeout from the normal non-evented client, a Mysql::Error is raised, but I am not sure it would be ok to raise that exception in this case.

I also understand that a change like this one would break compatibility. Maybe there is a better way to expose this timeout logic to the client code. On the other hand it is also a bit counter intuitive to have a read_timeout option that doesn't do anything when the client is evented :)

Please let me know what you think about it.
Have a nice day!

@gmodarelli
Copy link
Author

I think the failing test on Ruby 2.3.4 is just a random failure. I've run the tests locally with Ruby 2.3.4 and they are green

EM.stop_event_loop
end
end
end.to raise_error(Mysql2::EM::ReadTimeout)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you switch to the defer.errback pattern see in the tests below? It is problematic to throw exceptions outside the scope of EM.run (throwing exceptions like this in fact would crash EM 1.2.x until I finally finally figured out the problem in EM 1.2.6)

Copy link
Author

Choose a reason for hiding this comment

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

Sure, I'll switch to defer.errback :)

@sodabrew sodabrew added this to the 0.5.3 milestone Jul 4, 2018
@sodabrew sodabrew modified the milestones: 0.5.3, 0.6.0 Feb 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants