Fix lockyloo golang #8
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 3 commits
February 9, 2018 00:27
I believe this closes mattes#297 as well. I have been working on adding testing of migrations and it requires acquiring the lock in mysql multiple times to go up and down. After nailing this down to GET_LOCK returning a failure for every subsequent GET_LOCK call after the first, I decided it was time to rtfm and lo and behold: https://dev.mysql.com/doc/refman/5.7/en/miscellaneous-functions.html#function_release-lock RELEASE_LOCK will not work if called from a different thread than GET_LOCK. The simplest solution using the golang database/sql pkg appears to be to just get a single conn to use for every operation. since migrations are happening sequentially, I don't think this will be a performance hit (possible improvement). now calling Lock() and Unlock() multiple times will work; prior to this patch, every call to RELEASE_LOCK would fail. this required minimal changes to use the *sql.Conn methods instead of the *sql.DB methods. other changes: * upped time out to 10s on GET_LOCK, 1s timeout can too easily leave us in a state where we think we have the lock but it has timed out (during the operation). * fixes SetVersion which was not using the tx it was intending to, and fixed a bug where the transaction could have been left open since Rollback or Commit may never have been called. I added a test but it does not seem to come up in the previous patch, at least easily, I tried some shenanigans. At least, this behavior should be fixed with this patch and hopefully the test / comment is advisory enough. thank you for maintaining this library, it has been relatively easy to integrate with and most stuff works (pg works great :)
dhui
requested changes
Feb 14, 2018
database/mysql/mysql.go
Outdated
| db *sql.DB | ||
| // mysql RELEASE_LOCK must be called from the same conn, so | ||
| // just do everything over a single conn anyway. | ||
| db *sql.Conn |
| @@ -162,9 +172,9 @@ func (m *Mysql) Lock() error { | |||
| return err | |||
| } | |||
|
|
|||
| query := "SELECT GET_LOCK(?, 1)" | |||
| query := "SELECT GET_LOCK(?, 10)" | |||
There was a problem hiding this comment.
We should probably make the timeout configurable, but I think it's fine to make that change in a separate PR
dhui
approved these changes
Feb 14, 2018
|
go 1.10 is released bump :) woohoo, tests are fast now |
|
Fixed in release v3.2.0 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
source: mattes/migrate#299
got wind that this is the new happening place for development
Quick summary:
state where we think we have the lock but it has timed out (during the
operation).
bug where the transaction could have been left open since Rollback or Commit
may never have been called.
thanks for taking over maintenance of this project.