Repository navigation
Conversation
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.
|
Wow! This'll take me a bit to read through, thanks for the thorough research and PR! |
| } | ||
| } | ||
|
|
||
| static struct timeval *get_read_timeout(VALUE self, struct timeval *tvp) |
There was a problem hiding this comment.
When this function returns it returns the pointer from the tvp argument?
| @@ -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; | |||
There was a problem hiding this comment.
While you're in here, would you move the tv_usec = 0 to be directly below the tv_sec = sec line?
| * 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) ) { |
There was a problem hiding this comment.
Is has_data the only method that required the violite.h headers?
| int retval; | ||
|
|
||
| for(;;) { | ||
| retval = rb_wait_for_single_fd(fd, RB_WAITFD_IN, tvp); |
There was a problem hiding this comment.
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
| @@ -0,0 +1,280 @@ | |||
| /* Copyright (c) 2000, 2015, Oracle and/or its affiliates. All rights | |||
There was a problem hiding this comment.
Looking more closely, it's a little worse than just defining the one has_data method, we're actually becoming aware of the st_vio structure in order to call one of its member function pointers. I'm concerned that this isn't part of the "public API" and would be subject to change across MySQL versions.
There was a problem hiding this comment.
let's talk through this one before I address the rest of the stuff. Yes, you're definitely right that this isn't part of the public api; I'm not particularly happy with the solution, but I looked for quite a bit and couldn't find any function, public or not, that would advise you about whether you had to block.
Having said that, the history of this structure is that it hasn't had a breaking change since (maybe) 2104 or (probably) 2012: https://github.com/mysql/mysql-server/commits/5.7/include/violite.h
There was a problem hiding this comment.
the other possibility here is to declare dependency on the two functions that the has_next pointer could point to, vio_buff_has_data or vio_ssl_has_data, but this would mean figuring out whether the underlying connection has negotiated an SSL connection or not, and it's not clear to me if we can figure that out.
There was a problem hiding this comment.
Ok, that's a great lead. Let's see if there's a way to determine the connection type. It might be nice to expose that information, too like Mysql2::Client#ssl_connection?
|
A while back I thought about changing the multiple results API a bit to help support streaming on the 2nd - nth queries in a multiple statements, see #600 and let me know what you think of the idea proposed there. |
|
I want to pick up work on this PR again soon, but I prefer to bring it into a next major (0.5.0) rather than making major changes to 0.3.x or even 0.4.x now. Would you be amenable to rebasing your branch to master?
|
|
Closing in favor of rebase #962 |
…rianmario#1033 client.next_result calls mysql_next_result() directly. When the next statement in a multi-statement batch isn't ready yet, that call makes a real blocking network read -- unlike the initial query, which already releases the GVL and polls an interruptible fd wait (see do_query/wait_for_fd in client.c), next_result does neither. Bugs: - The GVL is held for the entire wait: a second Ruby thread gets zero scheduling opportunities while next_result blocks on a slow statement, confirmed by counting ticks from a sleeping counter thread during the wait. - The call is not interruptible: Timeout.timeout around next_result does not raise, because the underlying blocking C call can't respond to Thread#raise. The timeout is silently absorbed and the call returns normally after the full wait. Notes: - Both tests currently fail, by design -- that failure is the reproduction. They're written as assertions of correct behavior and are expected to start passing once a fix lands later on this same branch. - See the comment on brianmario#1033 for the full history: brianmario#807 (2016) and its rebase brianmario#962 (2018) attempted a fix via GVL release plus a vio-internals probe for already-buffered data, but neither merged. brianmario#962 is now closed as a historical artifact; this branch starts fresh from a reproduction, then a survey of what current libmysqlclient/MariaDB Connector/C offer for this.
…Thread#raise client.next_result called mysql_next_result() directly. When the next statement in a multi-statement batch (SELECT 1; SELECT SLEEP(3)) isn't ready yet, that's a real blocking network read -- unlike the initial query, which already releases the GVL and polls an interruptible fd wait (do_query/wait_for_fd), next_result did neither. brianmario#807 (2016) and its rebase brianmario#962 (2018) tried to fix this by reaching into vio_priv.h to detect already-buffered data before deciding whether to poll; neither merged, and the underlying bug (brianmario#1033) stayed open. Bugs: - The GVL was held for the entire wait: a second Ruby thread got zero scheduling opportunities while next_result blocked on a slow statement. - The call was not interruptible: Timeout.timeout around next_result did not raise, because the blocking C call couldn't respond to Thread#raise. The timeout was silently absorbed and the call returned normally after the full wait. Fixes: - On MySQL 8.0.16+, poll mysql_next_result_nonblocking() (a real libmysqlclient API added years after brianmario#807/brianmario#962 were written) instead of calling mysql_next_result() directly. Each iteration either completes immediately or reports NET_ASYNC_NOT_READY, in which case the wait -- not the poll -- is what actually blocks, via the same rb_wait_for_single_fd machinery do_query already uses, so it's interruptible the same way. Mixing this with the ordinary blocking API elsewhere on the same connection is explicitly documented as supported by MySQL, so nothing else in this file needed to change, and brianmario#962's vio-internals detection for already-buffered data is no longer needed at all -- the nonblocking function itself reports that. - Falls back to releasing the GVL around the still-blocking call (rb_thread_call_without_gvl) on MariaDB and on MySQL older than 8.0.16, guarded by have_func('mysql_next_result_nonblocking'). This fixes the GVL-holding half of the bug unconditionally; the interruptibility half depends on whether the linked libmysqlclient retries internally on the signal RUBY_UBF_IO sends on EINTR, which this fallback can't control. Verified empirically on this machine's MySQL 8.0.34 build with the fast path force-disabled: both problems actually resolved here too, but that isn't guaranteed everywhere. Notes: - Sets wrapper->state to QUERYING for the duration and reaps on the way out (rb_ensure), matching the QUERYING/IDLE bracket rb_mysql_query already uses. This didn't matter before: holding the GVL for the entire call incidentally serialized against any other thread's query/prepare/statement-close on this connection. Fixing the GVL-holding half of the bug removes that accidental protection, so the explicit bracket is now load-bearing, not just for consistency. - Added REQUIRE_CONNECTED, which the original function didn't have -- needed so a disconnected client can't reach the new poll loop with a stale/invalidated fd. - MariaDB Connector/C has its own non-blocking pair for this (mysql_next_result_start/_cont, confirmed to exist by reading Connector/C's actual header), not yet wired up here. Whether it's safely mixable with ordinary blocking calls the way MySQL's version is, or requires a connection-wide MYSQL_OPT_NONBLOCK commitment, is still being verified before extending this fix to that path. Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
2 participants
We were a multi-statement query in our, and discovered that when queries other than the first query blocked, (
SELECT 1; SELECT SLEEP(3)) the call tonext_resultbecame completely uninterruptible. This was fixed well enough by by wrapping thenext_resultcall inrb_thread_call_without_gvl, but we subsequently found that thenext_resultcall still wouldn't respond toThread#raise, which we needed in order to perform some timeout logic.Went down the rabbit hole further to find that the
viosuite of functions will over-read a buffer, so mysql2's methodology of callingrb_wait_for_single_fdreally won't work reliably fornext_result-- reading the first result in thedo_querypath will trigger a read of the result of the results (if they're ready), meaning that we'll poll forever; ugly. I worked around this by directly callingvio_has_data, but in order to do that I had to include all ofviolite.h, and I can't say I'm particularly happy about it.Anyway, we probably are simply backing off our using of multi-statement selects, if you wanted to merge this (or had better ideas about how to properly poll for results when the c client library had buffered stuff), great.
(note, PR targets 0.3 just because thats what we're on still).