with_advisory_lock
with_advisory_lock copied to clipboard
ClosureTree
Timeout not passed to MySQL timeout
Hey there -- thanks for providing this gem. Is there a reason you chose to always provide a timeout of 0 within the MySQL driver rather than pass the thread timeout along? Would you be receptive to a PR that changes this behavior?
Thanks!
Reopening-- @relwell, you're not on the hook for this. Anyone can submit a PR for it.
@mceachen Hi Matthew, I was looking into this but have a few questions (First Time contributor, so bare with me :)
It looks like the pg_try_advisory method of PostgresSQL does not support timeouts, see the documentation.
One idea is to make the implementation of the yield_with_lock method specific per database, i.e. move it to the respective database files. In this case then we can set a timeout for MySQL, which seems to be supported (see https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_get-lock) . But it feels like we are loosing genericity and introducing a more complex implementation. Is there anything i miss here? Thanks for any help/input :)
@serenaf thanks for looking into this. Your link to MySQL's fundamental change to get_lock (in a patch release, of course) is most unfortunate, and breaks this gem as it stands. So, good news that you highlighted that, bad news is there was something to highlight.
Just because Postgres doesn't support a timeout doesn't mean we can't support a timeout--we can do a polling retry with a fast-fail method (pg_try_advisory) for the duration of the timeout.
To be honest I'm not using ruby (or rails) for any projects (not for 4 years now!), and I've been supporting my ruby projects because there are too many abandoned libraries out there. If you'd like to take a stab at a PR, though, I'm happy to review it and help you get it merged, but there should be tests added to the matrix for MySQL < 5.7.5 and >= 5.7.5, and ActiveRecord 5.1 as well (given that is the currently supported version).
Howdy, @serenaf! Per @mceachen's recommendations, I added activerecord 5.1 and both flavors of MySQL (< 5.7.5 and >= 5.7.5) to the testing matrix. Would you be able to help craft a failing test so that we can get started on resolving?
Hi @mceachen. I know I'm 4 years late, but what do you mean by "breaks this gem as it stands"? I've tested locally with multiple threads trying to access the lock and everything looks fine. Is this gem safe to use in production?
@danielventulieri sorry, I don't know. This gem is being maintained by @seuros at this point, and I'll defer to his response.