Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Update file locking to use juju/mutex #19
Conversation
added some commits
Nov 9, 2016
cmars
approved these changes
Nov 14, 2016
bz2
approved these changes
Nov 14, 2016
Code changes look good to me.
If I understand earlier discussions correctly, we don't have anything to worry about with upgrades, as there's nothing persistent in the locking mechanism. Only shared jar with simultanious use by different juju versions would be an issue.
| -} | ||
| +const maxRetryDuration = 2 * time.Second | ||
| + | ||
| +var re = regexp.MustCompile(`[^a-z0-9A-Z]*`) |
bz2
Nov 14, 2016
Contributor
I'd name this something more descriptive given it's a top level variable (even though private).
| + } | ||
| + // We start with an alphabetical character, and remove non alphanumeric | ||
| + // characters so that we can't have an invalid name for the lock. | ||
| + final := re.ReplaceAllLiteralString(fmt.Sprintf("L%v%v", sha[:29], path), ``) |
bz2
Nov 14, 2016
•
Edited 1 time
-
bz2
Nov 14, 2016
Contributor
-
Nov 14, 2016
Didn't like encoding the hash to base64 then randomly chopping out characters, but given 62 are still valid I think it's actually fine. We have no need to try to match or compare the hash later.
voidspace
Nov 14, 2016
Contributor
It's certainly not ideal, but as only alphanumeric characters are valid for lock names the hash requires some kind of encoding. It will always be deterministic and the chances of collision are miniscule.
|
$$merge$$ |
voidspace commentedNov 14, 2016
•
Edited 1 time
-
voidspace
Nov 14, 2016
Change cookie file locking to no longer use file based locking and use juju/mutex instead. Also increase timeout and remove the exponential back-off when acquiring the lock.
This addresses Juju bug #1632362.
This change doesn't affect the API or the semantics and the cookie file locking is already tested with a concurrent access test, so there are no new tests.