Retry on "ActiveRecord::Deadlocked" errors (in addition to ActiveRecord::LockWaitTimeout)#7
Conversation
* Adding different tests for LockWaitTimeout error vs Deadlocked error. * Bumping gem to rails 7.1 so brakeman will pass. * Adding error class to log message so we can see which type of error was thrown. * The "innodb_status_cmd" was throwing errors in tests. * Added Installation instructions to README
Adding logs for when the max retries has been reached and when we are in a nested transaction. Switching the tests to non-regex matches, because they were getting more brittle with text escaping (brackets, paranthesis).
test/deadlock_retry_test.rb
Outdated
| DeadlockRetry.class_variable_set(:@@deadlock_logger_severity, nil) | ||
| end | ||
|
|
||
| def print_logs(logs) |
There was a problem hiding this comment.
This gem depends on rails. Should we just use Rails.logger?
Oh. Where is this method used?
There was a problem hiding this comment.
It's not used directly - it's a silly utility method I used when writing the specs. As I beefed up the logs, it became a bit harder to know "what is actually getting logged", so instead of constantly tailing log/test.log, I used this print function. I also ended up committing those logs as examples in the specs ie: here, because I wanted "documentation" on here's what the logs look like. (If you look at that spec, you'll see that it asserts the "nested transaction" line should appear 8 times, but it's hard to tell what order the lines occur in the logs. Rather then spending a bunch of time to assert which log entry exists in what order, I thought this was a quick way to at least document what Peter and others can see in the logs)
There was a problem hiding this comment.
I'm happy to delete it too
| DEPENDENCIES | ||
| activerecord (~> 6.0) | ||
| activerecord (~> 7.1.5.1) | ||
| byebug |
There was a problem hiding this comment.
The only reason we use byebug at all is because it works better with our selenium testing. If I could figure out how to make the standard debugger work well, we'd ditch it.
2 participants
Context: Before this PR, the gem was rescuing from
ActiveRecord::LockWaitTimeoutexceptions (which is good), butActiveRecord::Deadlockedexceptions were not getting caught (which is the issue). That means when our rails app is throwing aMysql2::Error: Deadlock found when trying to get lock; try restarting transactionerror, ourdeadlock_retrygem was not retrying the transaction.Fix: After this PR, we rescue from both exceptions todo the retry logic.
Notes:
The exceptions we are rescuing from are the same as the ones which Rails' built-in deadlock retry use (ala Rails 7.1): https://github.com/rails/rails/blob/e9d7ca5e0b834ff2ce3d43965deac670ff8ac790/activerecord/lib/active_record/connection_adapters/abstract_adapter.rb#L1079. This is good from a "we are retrying on the right things" and from a perspective that "hopefully we can cleanly migrate off this gem and onto Rails default behavior easily" in the near future.
Added some README directions for install/setup.
This PR should fix the dependabot security alerts: