Fix Windows flock/funlock race #122
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jstarks
reviewed
Sep 11, 2018
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.
Changed to _. Has to be in parameters to match other platform implementation though.
There was a problem hiding this comment.
I don't think it's used in any of the other platforms. It was just there for Windows.
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) |
lowenna
force-pushed
the
windowslocking
branch
from
acb75b1
to
f50e6c1
Compare
|
@jhowardmsft Thanks for fix. Can we add a test case? And any good recommendation for setting up Windows CI? |
lowenna
force-pushed
the
windowslocking
branch
from
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. |
lowenna
force-pushed
the
windowslocking
branch
from
cb7630c
to
1211141
Compare
lowenna
commented
Sep 11, 2018
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.
Oops. Will change that to a temp file. Had it hard coded in my test program.
There was a problem hiding this comment.
And that's fixed to use tempfile() instead.
Signed-off-by: John Howard <jhoward@microsoft.com>
lowenna
force-pushed
the
windowslocking
branch
from
1211141
to
9be7d8a
Compare
This was referenced Sep 11, 2018
|
@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 😄 |
|
@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. |
lowenna
pushed a commit
to lowenna/containerd
that referenced
this pull request
Sep 12, 2018
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
lowenna
pushed a commit
to microsoft/docker
that referenced
this pull request
Sep 13, 2018
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....
thaJeztah
pushed a commit
to thaJeztah/docker
that referenced
this pull request
Sep 14, 2018
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>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
Sep 14, 2018
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
dims
pushed a commit
to dims/sys
that referenced
this pull request
Oct 29, 2020
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....
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.