search

ClosureTree / with_advisory_lock

Advisory locking for ActiveRecord
http://closuretree.github.io/with_advisory_lock/
MIT License
619 stars 67 forks source link

Timeout not passed to MySQL timeout #17

Open relwell opened 8 years ago

relwell commented 8 years ago

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!

mceachen commented 7 years ago

Sure!

relwell commented 7 years ago

Cleaning out my open issues.

mceachen commented 7 years ago

Reopening-- @relwell, you're not on the hook for this. Anyone can submit a PR for it.

serenaf commented 7 years ago

@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 :)

mceachen commented 7 years ago

@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).

uhlenbrock commented 7 years ago

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?

dventulieri commented 2 years ago

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?

mceachen commented 2 years ago

@danielventulieri sorry, I don't know. This gem is being maintained by @seuros at this point, and I'll defer to his response.