-
Notifications
You must be signed in to change notification settings - Fork 1.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix concurrent access for SQL storage. #1370
base: master
Are you sure you want to change the base?
Changes from 2 commits
e8fe795
2d57695
8f68b89
ec4e637
2e9a63b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,10 +140,16 @@ func (c *conn) UpdateAuthRequest(id string, updater func(a storage.AuthRequest) | |
return err | ||
} | ||
|
||
err = c.flavor.lockForUpdate(tx, "auth_request", "id", r.ID) | ||
if err != nil { | ||
return fmt.Errorf("update auth request: %v", err) | ||
} | ||
|
||
a, err := updater(r) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
_, err = tx.Exec(` | ||
update auth_request | ||
set | ||
|
@@ -380,13 +386,23 @@ func (c *conn) UpdateKeys(updater func(old storage.Keys) (storage.Keys, error)) | |
firstUpdate := false | ||
// TODO(ericchiang): errors may cause a transaction be rolled back by the SQL | ||
// server. Test this, and consider adding a COUNT() command beforehand. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🤔 Is that comment still accurate now? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't remember. Ha |
||
old, err := getKeys(tx) | ||
old := storage.Keys{} | ||
err := c.flavor.lockForUpdate(tx, "keys", "id", keysRowID) | ||
if err != nil { | ||
if err != storage.ErrNotFound { | ||
return fmt.Errorf("get keys: %v", err) | ||
} | ||
|
||
firstUpdate = true | ||
old = storage.Keys{} | ||
} else { | ||
old, err = getKeys(tx) | ||
kepkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
if err != nil { | ||
if err != storage.ErrNotFound { | ||
return fmt.Errorf("get keys: %v", err) | ||
} | ||
|
||
firstUpdate = true | ||
} | ||
} | ||
|
||
nk, err := updater(old) | ||
|
@@ -457,6 +473,12 @@ func (c *conn) UpdateClient(id string, updater func(old storage.Client) (storage | |
if err != nil { | ||
return err | ||
} | ||
|
||
err = c.flavor.lockForUpdate(tx, "client", "id", id) | ||
if err != nil { | ||
return fmt.Errorf("update client: %v", err) | ||
} | ||
|
||
nc, err := updater(cli) | ||
if err != nil { | ||
return err | ||
|
@@ -577,6 +599,11 @@ func (c *conn) UpdatePassword(email string, updater func(p storage.Password) (st | |
return err | ||
} | ||
|
||
err = c.flavor.lockForUpdate(tx, "password", "email", p.Email) | ||
if err != nil { | ||
return fmt.Errorf("update password: %v", err) | ||
} | ||
|
||
np, err := updater(p) | ||
if err != nil { | ||
return err | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,6 +22,10 @@ type flavor struct { | |
// Optional function to create and finish a transaction. | ||
executeTx func(db *sql.DB, fn func(*sql.Tx) error) error | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This field could go away now, can't it? |
||
|
||
// Function for locking rows. It must return error if locking failed. If underlying | ||
kepkin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// database doesn't support locking (sqlite3) just make an empty implementation. | ||
lockForUpdate func(tx *trans, table, column, value string) error | ||
|
||
// Does the flavor support timezones? | ||
supportsTimezones bool | ||
} | ||
|
@@ -43,27 +47,9 @@ var ( | |
// The "github.com/lib/pq" driver is the default flavor. All others are | ||
// translations of this. | ||
flavorPostgres = flavor{ | ||
// The default behavior for Postgres transactions is consistent reads, not consistent writes. | ||
// For each transaction opened, ensure it has the correct isolation level. | ||
// | ||
// See: https://www.postgresql.org/docs/9.3/static/sql-set-transaction.html | ||
// | ||
// NOTE(ericchiang): For some reason using `SET SESSION CHARACTERISTICS AS TRANSACTION` at a | ||
// session level didn't work for some edge cases. Might be something worth exploring. | ||
executeTx: func(db *sql.DB, fn func(sqlTx *sql.Tx) error) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does anything use this anymore? can we nuke it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nuked |
||
tx, err := db.Begin() | ||
if err != nil { | ||
return err | ||
} | ||
defer tx.Rollback() | ||
|
||
if _, err := tx.Exec(`SET TRANSACTION ISOLATION LEVEL SERIALIZABLE;`); err != nil { | ||
return err | ||
} | ||
if err := fn(tx); err != nil { | ||
return err | ||
} | ||
return tx.Commit() | ||
lockForUpdate: func(tx *trans, table, column, value string) error { | ||
_, err := tx.Exec("SELECT 1 FROM "+table+" WHERE "+column+" = $1 FOR UPDATE NOWAIT;", value) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (For my own understanding,) So this doesn't attempt to address #1341 (and associated PR #1342) as-is; that could be done in a separate PR and would also involve changes to the tests. I suppose that could either be done by removing the I'm happy to submit that PR after this is merged if no one else has plans to already, not trying to increase scope of your PR. Just making sure I understand. 🙂 Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably better to add an inline comment :) This works, but the lockForUpdate API seems a bit risky. Ideally it could be baked into ExecTx or something, but nothing's coming to mind at the moment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes NOWAIT here is only because of tests. And yes, I think tests need to be redesigned a little bit, as changing the same auth_request concurrently wouldn't actually harm a login flow, and deadlock which current tests produce is not possible in real life situations. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should probably just update/remove the tests then, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I haven't come to idea how to test concurrency better yet. Maybe later. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about the issue with interleaving connectors in the concurrent login flow mentioned in #1356 (comment) ? Failing one of the transactions seems to be the only way to handle it in current implementation of handler/storage. |
||
return err | ||
}, | ||
|
||
supportsTimezones: true, | ||
|
@@ -82,6 +68,11 @@ var ( | |
// SQLite doesn't have a "now()" method, replace with "date('now')" | ||
{regexp.MustCompile(`\bnow\(\)`), "date('now')"}, | ||
}, | ||
|
||
// There is no requirement for concurrent access for SQLite3 | ||
lockForUpdate: func(tx *trans, table, column, value string) error { | ||
return nil | ||
}, | ||
} | ||
) | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💭 All these
fmt.Errorf
make me nervous -- they're hard to deal with in call sites -- but this is the style this package is written in, so it's fine at add a few. 👍There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can switch to github.com/pkg/errors in another PR :)