-
Notifications
You must be signed in to change notification settings - Fork 628
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
Support no timeout locks on db files #35
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
+ Coverage 85.01% 85.56% +0.55%
==========================================
Files 9 9
Lines 1848 1857 +9
==========================================
+ Hits 1571 1589 +18
+ Misses 164 159 -5
+ Partials 113 109 -4
Continue to review full report at Codecov.
|
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.
the duplicated code looks like it can be simplified some
db.go
Outdated
@@ -40,6 +40,9 @@ const ( | |||
// default page size for db is set to the OS page size. | |||
var defaultPageSize = os.Getpagesize() | |||
|
|||
// The step used by the timeout mechanisms. | |||
const timeoutStep = 50 * time.Millisecond |
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.
const flockRetryTimeout = 50 * time.Millisecond
?
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.
Good for me. Will push an update
bolt_unix_solaris.go
Outdated
if err == nil { | ||
return nil | ||
} else if err != syscall.EAGAIN { | ||
return err | ||
} | ||
|
||
// If we timed out then return an error. | ||
if timeout != 0 && (maxSleep < 0 || time.Since(t) > maxSleep) { |
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.
can get rid of maxSleep:
func flock(...) error {
t := time.Now()
fd := db.file.Fd()
...
if timeout != 0 && time.Since(t) > (timeout - flockRetryTimeout) {
return ErrTimeout
}
time.Sleep(flockRetryTimeout)
}
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.
The goal of the initial pull request was specifically to allow the API user to not depend upon any time logic - return directly with an error if the database file cannot be locked, without depending on the OS time settings.
That proposed code change would specifically create a hidden dependency with the time module. I checked it for correctness, it would work under Go 1.9 which brings monotonic guarantees. However under previous Go version, there are no monotonic guarantees and the code could result in a time.Sleep call (e.g., if the OS time is changed for whatever reason).
So it boils down to which version of Go do we support to guarantee that boltdb work. Do we want this patch to work for Go versions before 1.9 too, or do we design only for the latest version of Go? In the latter case, fine for me, I can push an update.
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.
I think relying on >=1.9 is OK since this is a new feature that won't interfere with old behavior.
I implemented the proposed changes |
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.
lgtm after squashing, thanks!
Will you use github's Squash and merge feature to squash the commits? |
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.
lgtm.
This is continuation of pull request #34 with same name, itself derived from boltdb/bolt#494
I implemented the changes proposed by @heyitsanthony. I ended up with conflicts between my version derived from boltdb/bolt and the coreos/bbolt master. I resolved them and committed them into a clean branch based on coreos/bbolt master