|
|
DescriptionProvide a file system level lock.
In order to implement hook serialisation we need a way to be able to
synchronise between processes. I stole from bzrlib here the concept of using
lock directories. Directories are used as renames on directories are
implemented as atomic operations at the file system level for all known
filesystems, so if two processes try to do this at the same time, only one
will succeed. This is the basis of the lockdir implementation.
A slightly over-engineered approach here as it will also work on windows where
flock doesn't exist.
https://code.launchpad.net/~thumper/juju-core/lockdir/+merge/159078
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 42
Patch Set 2 : Provide a file system level lock. #
Total comments: 8
MessagesTotal messages: 5
Please take a look.
Sign in to reply to this message.
great direction, but could use some work. lots of comments but nothing substantial. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go File utils/lockdir/lockdir.go (right): https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode9 utils/lockdir/lockdir.go:9: // won.) I wonder if we should call this package "fslock" and keep hidden the fact that it's using directories for the implementation. Then we can easily change the implementation if we find there are performance issues (for instance we could probe the directory to find whether advisory locks work). Also, this comment would work well as a doc comment if you removed the blank line below. BTW is the "some filesystems or transports only have rename-and-overwrite" really an issue for us? os provides O_EXCL and creating a file is usually less work than creating a directory, i think. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode26 utils/lockdir/lockdir.go:26: InvalidLockName = errors.New("Lock names must match regex `^[a-z]+[a-z0-9.-]*$") i think it would be more useful to include the actual bad name in the error message than the regex. "invalid lock name %q" otherwise i'd put the regex string into a constant and derive this error and validName from that to avoid the duplication. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode27 utils/lockdir/lockdir.go:27: LockNotHeld = errors.New("Lock not held") errors variables are conventionally named with a leading "Err". so this would be ErrLockHeld. We tend to leave errors in lower case too. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode34 utils/lockdir/lockdir.go:34: type Lock struct { this needs a doc comment. also, for consistency with the sync package, perhaps we should call this "Mutex". // Mutex represents a file-system based mutual exclusion lock. ? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode36 utils/lockdir/lockdir.go:36: lockDir string this is a slightly misleading name, because all the locks are directories. "parent" ? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode40 utils/lockdir/lockdir.go:40: func GenerateNonce() (string, error) { does this need to be exported? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode41 utils/lockdir/lockdir.go:41: const size = 20 i don't think this is worth it (see below) https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode42 utils/lockdir/lockdir.go:42: var nonce [size]byte given that you only ever use it as a slice, you may as well do nonce := make([]byte, 20) https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode43 utils/lockdir/lockdir.go:43: if _, err := io.ReadFull(rand.Reader, []byte(nonce[0:size])); err != nil { s/[]byte(nonce[0:size])/nonce[:]/ (if you keep the array) https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode49 utils/lockdir/lockdir.go:49: // Return a new lock. // NewLock returns a new lock with the given name within // the given lock directory, without acquiring it. // The lock name must match the regular expression // `^[a-z]+[a-z0-9.-]*`. ? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode52 utils/lockdir/lockdir.go:52: if !validName.MatchString(name) { do this before generating the nonce? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode63 utils/lockdir/lockdir.go:63: // Ensure the lockDir exists. if err := os.MkdirAll(lock.lockDir, 0755); err != nil { return nil, err } is all you need for the rest of this function. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode87 utils/lockdir/lockdir.go:87: func (lock *Lock) namedLockDir() string { if we rename Lock.lockDir to "parent" then this function could be renamed "lockDir". https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode97 utils/lockdir/lockdir.go:97: dir, err := os.Open(lock.namedLockDir()) i'd use os.Stat; then you don't need to close anything. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:106: tempDirName, err := ioutil.TempDir("", "temp-lock") rather than using TempDir (which isn't great with lots of competing clients) could we not just name the temp dir using the nonce (b16 encoded or whatever) ? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:119: err = ioutil.WriteFile(lock.heldFile(), []byte(lock.nonce), 0755) just a thought - if we're only ever using lock.nonce as a []byte, why not store it in the Lock as that type? (you can use bytes.Equal to compare byte slices) https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:140: return nil // unreachable d (the compiler knows about panic) https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:143: func (lock *Lock) TryLock(duration time.Duration) (isLocked bool, err error) { in my experience, TryLock usually means "try to take a lock and fail immediately if it's taken" - essentially the functionality of acquire above. i'd name this function LockWithTimeout; or just add a timeout duration argument to Lock and specify that if it's zero it waits forever. as for this function, AFAICS the goroutine is overkill. for example: // LockWithTimeout tries to acquire the lock. If it cannot // acquire the lock within the given duration, it // returns ErrTimeout. func (lock *Lock) LockWithTimeout(duration time.Duration) error { deadline := time.Now().Add(duration) for { acquired, err := lock.acquire() if err != nil { return err } if acquired { return nil } if time.Now.After(deadline) { return ErrTimeout } time.Sleep(lockWaitDelay) } panic("unreachable") } https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:190: // file 'held' in that directory contains the nonce for this lock. this sounds like an implementation-internal comment. and i'm not sure about the name - we know it's a lock. how about: // IsHeld returns whether the lock is currently held // by the receiver. ? https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:204: } i wonder if we might want Break too. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir_test.go File utils/lockdir/lockdir_test.go (right): https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir_test.go#n... utils/lockdir/lockdir_test.go:53: invalidLockName("no$dollar") i'd make this a table-driven test: var namedLockDirTests = []struct { name string valid bool } { {"a", true}, {"longer", true}, {"NoCapitals", false}, etc } or one table ([]string) for valid names and one for invalid names. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir_test.go#n... utils/lockdir/lockdir_test.go:121: // Waiting for something not to happen is inherintly hard... s/inherintly/inherently/
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go File utils/lockdir/lockdir.go (right): https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode9 utils/lockdir/lockdir.go:9: // won.) On 2013/04/16 08:03:43, rog wrote: > I wonder if we should call this package "fslock" and keep hidden the fact that > it's using directories for the implementation. Then we can easily change the > implementation if we find there are performance issues (for instance we could > probe the directory to find whether advisory locks work). > > Also, this comment would work well as a doc comment if you removed the blank > line below. This comment was supposed to be a doc comment. Didn't realise that I had to remove the blank line. > BTW is the "some filesystems or transports only have rename-and-overwrite" > really an issue for us? os provides O_EXCL and creating a file is usually less > work than creating a directory, i think. I'm happy with fslock. The main reason I went with directories is the same reason that bzrlib did. Robert Collins (lifeless) did a lot of research into this, and I'm just leveraging off what they did. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode26 utils/lockdir/lockdir.go:26: InvalidLockName = errors.New("Lock names must match regex `^[a-z]+[a-z0-9.-]*$") On 2013/04/16 08:03:43, rog wrote: > i think it would be more useful to include the actual > bad name in the error message than the regex. > > "invalid lock name %q" > > otherwise i'd put the regex string into a constant and derive this error and > validName from that to avoid the duplication. Yeah, this is one of those things that as I was writing it I thought "shouldn't really do this". Have moved it to fmt.Errorf now. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode27 utils/lockdir/lockdir.go:27: LockNotHeld = errors.New("Lock not held") On 2013/04/16 08:03:43, rog wrote: > errors variables are conventionally named with a leading "Err". > > so this would be ErrLockHeld. > > We tend to leave errors in lower case too. Done. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode34 utils/lockdir/lockdir.go:34: type Lock struct { On 2013/04/16 08:03:43, rog wrote: > this needs a doc comment. > > also, for consistency with the sync package, perhaps > we should call this "Mutex". > > // Mutex represents a file-system based mutual exclusion lock. > > ? Very -1 on calling this Mutex. Assumptions are a dangerous thing. While there are similarities, they are different. Mutex methods don't return errors. I'd be happier keeping this as fslock.Lock. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode36 utils/lockdir/lockdir.go:36: lockDir string On 2013/04/16 08:03:43, rog wrote: > this is a slightly misleading name, because all the locks are directories. > "parent" ? Sure. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode40 utils/lockdir/lockdir.go:40: func GenerateNonce() (string, error) { On 2013/04/16 08:03:43, rog wrote: > does this need to be exported? No. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode42 utils/lockdir/lockdir.go:42: var nonce [size]byte On 2013/04/16 08:03:43, rog wrote: > given that you only ever use it as a slice, you may as well do nonce := > make([]byte, 20) Yeah, this is cleaner, done. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode43 utils/lockdir/lockdir.go:43: if _, err := io.ReadFull(rand.Reader, []byte(nonce[0:size])); err != nil { On 2013/04/16 08:03:43, rog wrote: > s/[]byte(nonce[0:size])/nonce[:]/ > > (if you keep the array) Done. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode49 utils/lockdir/lockdir.go:49: // Return a new lock. On 2013/04/16 08:03:43, rog wrote: > // NewLock returns a new lock with the given name within > // the given lock directory, without acquiring it. > // The lock name must match the regular expression > // `^[a-z]+[a-z0-9.-]*`. Looks good. Using. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode52 utils/lockdir/lockdir.go:52: if !validName.MatchString(name) { On 2013/04/16 08:03:43, rog wrote: > do this before generating the nonce? Makes sense. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode63 utils/lockdir/lockdir.go:63: // Ensure the lockDir exists. On 2013/04/16 08:03:43, rog wrote: > if err := os.MkdirAll(lock.lockDir, 0755); err != nil { > return nil, err > } > > is all you need for the rest of this function. That is much simpler... https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode87 utils/lockdir/lockdir.go:87: func (lock *Lock) namedLockDir() string { On 2013/04/16 08:03:43, rog wrote: > if we rename Lock.lockDir to "parent" then this function could be renamed > "lockDir". Yep, simpler and changed. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcode97 utils/lockdir/lockdir.go:97: dir, err := os.Open(lock.namedLockDir()) On 2013/04/16 08:03:43, rog wrote: > i'd use os.Stat; then you don't need to close anything. I was looking for a function like that, but it was grouped in the docs under type FileInfo, and I didn't notice it. Changed. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:106: tempDirName, err := ioutil.TempDir("", "temp-lock") On 2013/04/16 08:03:43, rog wrote: > rather than using TempDir (which isn't great with lots of competing clients) > could we not just name the temp dir using the nonce (b16 encoded or whatever) ? How about a compromise, and use the TempDir with the encoded nonce? Although the level of actual work is pretty trivial. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:119: err = ioutil.WriteFile(lock.heldFile(), []byte(lock.nonce), 0755) On 2013/04/16 08:03:43, rog wrote: > just a thought - if we're only ever using lock.nonce as a []byte, why not store > it in the Lock as that type? > (you can use bytes.Equal to compare byte slices) Yep, updated. I had asked how to compare two byte slices and was told to use reflect.DeepEquals, which seemed a little weird. But happy to use bytes.Equal. Reviews are a great source of learning. Thanks. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:140: return nil // unreachable On 2013/04/16 08:03:43, rog wrote: > d > (the compiler knows about panic) OK. I must have put the return there first, then added the panic. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:143: func (lock *Lock) TryLock(duration time.Duration) (isLocked bool, err error) { On 2013/04/16 08:03:43, rog wrote: > in my experience, TryLock usually means "try to take a lock and fail immediately > if it's taken" - essentially the functionality of acquire above. > > i'd name this function LockWithTimeout; or just add a timeout duration argument > to Lock and specify that if it's zero it waits forever. Fair enough. > as for this function, AFAICS the goroutine is overkill. I agree. I thought there must be an easier way, but then I wanted to see if I could actually get it working. Was a little surprised when it did work, but then I didn't refactor. Using your simpler code. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:190: // file 'held' in that directory contains the nonce for this lock. On 2013/04/16 08:03:43, rog wrote: > this sounds like an implementation-internal comment. > and i'm not sure about the name - we know it's a lock. > > how about: > > // IsHeld returns whether the lock is currently held > // by the receiver. > > ? Sounds good. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir.go#newcod... utils/lockdir/lockdir.go:204: } On 2013/04/16 08:03:43, rog wrote: > i wonder if we might want Break too. I was wondering about that too. Given that we don't have any way to explicitly break the lock I was thinking of the following: // GetAge returns the duration the lock has currently been held. func (lock *Lock) GetAge() *time.Duration // should be able to determine this from time.Now() and the os.Stat from the created directory. // BreakLock forcefully breaks the existing lock. func (lock *Lock) BreakLock() error In order to aid debugging, I was wondering about adding the following two as well: func (lock *Lock) SetMessage(message string) error func (lock *Lock) GetMessage() string which would write the message to the lockdir/message file (if and only if the lock is held). This message could be queried by any other lock holder, and used in conjunction with GetAge and BreakLock if necessary for logging. Also gives us the opportunity to ssh into the machine and look at the lock directories to see what was holding the lock if things go wrong. https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir_test.go File utils/lockdir/lockdir_test.go (right): https://codereview.appspot.com/8602046/diff/1/utils/lockdir/lockdir_test.go#n... utils/lockdir/lockdir_test.go:53: invalidLockName("no$dollar") On 2013/04/16 08:03:43, rog wrote: > i'd make this a table-driven test: > > var namedLockDirTests = []struct { > name string > valid bool > } { > {"a", true}, > {"longer", true}, > {"NoCapitals", false}, > etc > } > > or one table ([]string) for valid names and one for invalid names. Done.
Sign in to reply to this message.
Definitely need clarity on the second point below. https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:52: func NewLock(lockDir, name string) (*Lock, error) { s/lockDir/parent/ here too? https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:95: err = os.Rename(tempDirName, lock.lockDir()) Shouldn't we have the held file in place before we try to overwrite? (This is based on jam's comment about renaming over the top of empty directories; I am ignorant here.) https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:122: time.Sleep(lockWaitDelay) log.Debugf something? possibly even also a "lock held by XXX" once we have agreement on Message (or rather, on setting the message on creation, or whatever we decide) https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock_test.go File utils/fslock/fslock_test.go (right): https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock_test.go#... utils/fslock/fslock_test.go:52: "no$dollar", no:colon, maybe?
Sign in to reply to this message.
Addressed these issues. I've added a new pipe which now contains all the synchronize work, and we can have one final review of all that at the end. The stress test actually found two errors in the code, which are now fixed \o/ https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go File utils/fslock/fslock.go (right): https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:52: func NewLock(lockDir, name string) (*Lock, error) { On 2013/04/17 14:04:29, fwereade wrote: > s/lockDir/parent/ here too? Nah. I kind of like the parameter name as it actually says what it is. "parent" isn't descriptive. lockDir is the directory where the locks go. https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:95: err = os.Rename(tempDirName, lock.lockDir()) On 2013/04/17 14:04:29, fwereade wrote: > Shouldn't we have the held file in place before we try to overwrite? (This is > based on jam's comment about renaming over the top of empty directories; I am > ignorant here.) Yes we should, and this is now done. https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock.go#newco... utils/fslock/fslock.go:122: time.Sleep(lockWaitDelay) On 2013/04/17 14:04:29, fwereade wrote: > log.Debugf something? possibly even also a "lock held by XXX" once we have > agreement on Message (or rather, on setting the message on creation, or whatever > we decide) Yep, added some. https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock_test.go File utils/fslock/fslock_test.go (right): https://codereview.appspot.com/8602046/diff/6001/utils/fslock/fslock_test.go#... utils/fslock/fslock_test.go:52: "no$dollar", On 2013/04/17 14:04:29, fwereade wrote: > no:colon, maybe? Sure, why not.
Sign in to reply to this message.
|