-
Notifications
You must be signed in to change notification settings - Fork 634
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 Windows flock/funlock race #122
Conversation
bolt_windows.go
Outdated
@@ -49,27 +49,23 @@ func fdatasync(db *DB) error { | |||
|
|||
// flock acquires an advisory lock on a file descriptor. | |||
func flock(db *DB, mode os.FileMode, exclusive bool, timeout time.Duration) error { |
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.
mode
is probably not used anymore.
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.
Changed to _
. Has to be in parameters to match other platform implementation though.
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 don't think it's used in any of the other platforms. It was just there for Windows.
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.
Ah, indeed. Removed.
bolt_windows.go
Outdated
// Create a separate lock file on windows because a process | ||
// cannot share an exclusive lock on the same file. This is | ||
// needed during Tx.WriteTo(). | ||
f, err := os.OpenFile(db.path+lockExt, os.O_CREATE, mode) |
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.
lockExt
's definition can be removed
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 catch. Updated.
acb75b1
to
f50e6c1
Compare
@jhowardmsft Thanks for fix. Can we add a test case? And any good recommendation for setting up Windows CI? |
f50e6c1
to
cb7630c
Compare
Sure, added TestOpen_MultipleGoroutines. Basically that same test code I was using, doing 30 iterations of 30 goroutines. It doesn't look like Travis supports Windows yet, although there's many requests for it as far as I can see from a quick look: travis-ci/travis-ci#2104 as an example. However, appveyor does support Windows - it's what we have for CI in https://github.com/Microsoft/hcsshim for example. It also looks like Azure Devops/Azure pipeline may be free: https://azure.microsoft.com/en-us/services/devops/pipelines/ New test:
And the entire test suite locally, including that new test.
|
cb7630c
to
1211141
Compare
db_test.go
Outdated
wg.Add(1) | ||
go func() { | ||
defer wg.Done() | ||
db, err := bolt.Open(`c:\bolt\test.db`, 0600, 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.
Oops. Will change that to a temp file. Had it hard coded in my test program.
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.
And that's fixed to use tempfile() instead.
Signed-off-by: John Howard <jhoward@microsoft.com>
1211141
to
9be7d8a
Compare
@gyuho Are you OK merging this and tagging a release? I have some 7 upstream projects currently using boltdb/bolt which I'd like to get all moved across to etcd-io/bbolt and pick up this fix. Thanks 😄 |
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 should be safe since mode
arg was never used for flock
unix builds. /cc @xiang90
@jhowardmsft I will let @xiang90 do the final review. And as soon as it gets merged, I will cut another release. Thanks for fix! |
Not a windows expert. But the fix and the test make sense to me. |
@jhowardmsft Tagging now. |
Fantastic. Thank you so much. |
Signed-off-by: John Howard <jhoward@microsoft.com> This is the maintained version of boltdb, which includes the Windows-specific fix detailed in etcd-io/bbolt#122
Signed-off-by: John Howard <jhoward@microsoft.com> This also adds go.etcd.io/bbolt as boltdb/bolt is no longer maintained, and we need etcd-io/bbolt#122 which was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8 in order to fix moby/libnetwork#1950. Note that I can't entirely remove boltdb/bolt as it is still used by other components. Still need to work my way through them.... These include containerd/containerd (containerd/containerd#2634), docker/swarmkit; moby/buildkit. And probably more....
Signed-off-by: John Howard <jhoward@microsoft.com> This also adds go.etcd.io/bbolt as boltdb/bolt is no longer maintained, and we need etcd-io/bbolt#122 which was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8 in order to fix moby/libnetwork#1950. Note that I can't entirely remove boltdb/bolt as it is still used by other components. Still need to work my way through them.... These include containerd/containerd (containerd/containerd#2634), docker/swarmkit; moby/buildkit. And probably more.... (cherry picked from commit 1a6e260) Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: John Howard <jhoward@microsoft.com> This also adds go.etcd.io/bbolt as boltdb/bolt is no longer maintained, and we need etcd-io/bbolt#122 which was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8 in order to fix moby/libnetwork#1950. Note that I can't entirely remove boltdb/bolt as it is still used by other components. Still need to work my way through them.... These include containerd/containerd (containerd/containerd#2634), docker/swarmkit; moby/buildkit. And probably more.... Upstream-commit: 1a6e2609ead86144b75067bfe5154dad5e42d5cf Component: engine
Signed-off-by: John Howard <jhoward@microsoft.com> This also adds go.etcd.io/bbolt as boltdb/bolt is no longer maintained, and we need etcd-io/bbolt#122 which was merged in https://github.com/etcd-io/bbolt/releases/tag/v1.3.1-etcd.8 in order to fix moby/libnetwork#1950. Note that I can't entirely remove boltdb/bolt as it is still used by other components. Still need to work my way through them.... These include containerd/containerd (containerd/containerd#2634), docker/swarmkit; moby/buildkit. And probably more....
Signed-off-by: John Howard jhoward@microsoft.com
Fixes #121, and downstream moby/libnetwork#1950.
Using the test program from #121:
Running it in two separate Windows, this now succeeds after running a significant amount of time without a panic. Pre this change, it would panic within the first few iterations.
$lastExitCode=0;$i=0;while ($lastExitCode -eq 0) {$i++;echo $i;.\test.exe}
etc.
@jstarks - Could you do a quick check please.
@dineshgovindasamy @msabansal @carlfischer1 @johnstep @mavenugo FYI.