Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1563)

Issue 8602046: Provide a file system level lock.

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by thumper
Modified:
9 years, 7 months ago
Reviewers:
mp+159078, fwereade, rog
Visibility:
Public.

Description

Provide 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+351 lines, -0 lines) Patch
A [revision details] View 1 1 chunk +2 lines, -0 lines 0 comments Download
A utils/fslock/export_test.go View 1 1 chunk +10 lines, -0 lines 0 comments Download
A utils/fslock/fslock.go View 1 1 chunk +163 lines, -0 lines 6 comments Download
A utils/fslock/fslock_test.go View 1 1 chunk +176 lines, -0 lines 2 comments Download

Messages

Total messages: 5
thumper
Please take a look.
9 years, 7 months ago (2013-04-16 05:29:37 UTC) #1
rog
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 ...
9 years, 7 months ago (2013-04-16 08:03:43 UTC) #2
thumper
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, ...
9 years, 7 months ago (2013-04-16 23:07:37 UTC) #3
fwereade
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#newcode52 utils/fslock/fslock.go:52: func ...
9 years, 7 months ago (2013-04-17 14:04:29 UTC) #4
thumper
9 years, 7 months ago (2013-04-18 02:00:44 UTC) #5
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b