{"action":"created","comment":{"url":"https://api.github.com/repos/coreos/rkt/pulls/comments/58932868","id":58932868,"diff_hunk":"@@ -49,15 +51,17 @@ func newDBLock(dirPath string) (*dbLock, error) {\n }\n \n func (dl *dbLock) lock() error {\n+\tdl.Lock()\n \tif err := dl.fl.ExclusiveLock(); err != nil {\n+\t\tdl.Unlock()\n \t\treturn err\n \t}\n-\tdl.Lock()\n \treturn nil\n }\n \n func (dl *dbLock) unlock() error {\n \tif err := dl.fl.Unlock(); err != nil {\n+\t\tdl.Unlock()\n \t\treturn err\n \t}\n \tdl.Unlock()","path":"store/db.go","position":32,"original_position":32,"commit_id":"78fc8d5ab8679d8793eef2d36d6956a10b686da8","original_commit_id":"78fc8d5ab8679d8793eef2d36d6956a10b686da8","user":{"login":"sgotti","id":1690273,"avatar_url":"https://avatars.githubusercontent.com/u/1690273?v=3","gravatar_id":"","url":"https://api.github.com/users/sgotti","html_url":"https://github.com/sgotti","followers_url":"https://api.github.com/users/sgotti/followers","following_url":"https://api.github.com/users/sgotti/following{/other_user}","gists_url":"https://api.github.com/users/sgotti/gists{/gist_id}","starred_url":"https://api.github.com/users/sgotti/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgotti/subscriptions","organizations_url":"https://api.github.com/users/sgotti/orgs","repos_url":"https://api.github.com/users/sgotti/repos","events_url":"https://api.github.com/users/sgotti/events{/privacy}","received_events_url":"https://api.github.com/users/sgotti/received_events","type":"User","site_admin":false},"body":"> why not just call dl.Unlock() at the top of the function?\r\n\r\nIf your question is why I inverted the order putting the sync.Mutex lock before the flock that's because the sync.Mutex must protect the flock. The reason is that calling flock.ExclusiveLock after a previous ExclusiveLock will continue since it's on the same filedescriptor. This can cause something like this:\r\n\r\n* Process A, goroutine 1: takes the the flock, then takes the lock on the mutex, and do db.Open\r\n* Process A, goroutine 2: takes the flock, blocks on the lock on the mutex\r\n* Process A, goroutine 1: does something on db, calls db.Close, release the lock on the mutex, unlocks the flock\r\n* Process A, goroutine 2: takes the lock on the mutex, but now it has the flock UNLOCKED\r\n* Process B, goroutine 1: take the lock on the mutex (since it's another process), and takes the lock on the flock: the ql camlistore lock will fail.\r\n\r\nSo the sync.Mutex that's really (per store instance) exclusive must go before the flock\r\n\r\nObviously there must be only one store instance (on the same dir) per process. This is what actually rkt does (I haven't found wrong store usage).","created_at":"2016-04-07T19:45:17Z","updated_at":"2016-04-07T19:45:17Z","html_url":"https://github.com/coreos/rkt/pull/2391#discussion_r58932868","pull_request_url":"https://api.github.com/repos/coreos/rkt/pulls/2391","_links":{"self":{"href":"https://api.github.com/repos/coreos/rkt/pulls/comments/58932868"},"html":{"href":"https://github.com/coreos/rkt/pull/2391#discussion_r58932868"},"pull_request":{"href":"https://api.github.com/repos/coreos/rkt/pulls/2391"}}},"pull_request":{"url":"https://api.github.com/repos/coreos/rkt/pulls/2391","id":65654874,"html_url":"https://github.com/coreos/rkt/pull/2391","diff_url":"https://github.com/coreos/rkt/pull/2391.diff","patch_url":"https://github.com/coreos/rkt/pull/2391.patch","issue_url":"https://api.github.com/repos/coreos/rkt/issues/2391","number":2391,"state":"open","locked":false,"title":"store: fix goroutines race on db.","user":{"login":"sgotti","id":1690273,"avatar_url":"https://avatars.githubusercontent.com/u/1690273?v=3","gravatar_id":"","url":"https://api.github.com/users/sgotti","html_url":"https://github.com/sgotti","followers_url":"https://api.github.com/users/sgotti/followers","following_url":"https://api.github.com/users/sgotti/following{/other_user}","gists_url":"https://api.github.com/users/sgotti/gists{/gist_id}","starred_url":"https://api.github.com/users/sgotti/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgotti/subscriptions","organizations_url":"https://api.github.com/users/sgotti/orgs","repos_url":"https://api.github.com/users/sgotti/repos","events_url":"https://api.github.com/users/sgotti/events{/privacy}","received_events_url":"https://api.github.com/users/sgotti/received_events","type":"User","site_admin":false},"body":"When a process using, from multiple goroutines, the same store.db instance\r\nit'll happen that a goroutine will Open the ql db with another\r\ngoroutine/process with it already open.\r\n\r\nThis is caused because in db.Do the store.db instance is using one\r\nfile lock (using flock) so a following call to db.Do with an already held\r\nlock will pass and call Open.\r\nWith a single process this isn't causing an error probably because,\r\nsince ql uses fnctl, the posix lock is related to the file and process\r\nand not to the file descriptor (so this can cause a real db corruption).\r\n\r\nInstead when using more than one process this will trigger the ql\r\nlocking error: cannot acquire lock: resource temporarily unavailable\r\n\r\nIn the code there was already a internal locking made by a mutex, but\r\nlooks like it was put just to make the go race detector happy and the\r\nlock/unlock order was wrong. Instead this is really needed.\r\n\r\nThis patch fixes the lock/unlock order.\r\n\r\nA future test to really test this problem will need to spawn two\r\nprocesses.\r\n\r\nHere an example repro:\r\n\r\n```\r\npackage main\r\n\r\nimport (\r\n        \"fmt\"\r\n        \"os\"\r\n\r\n        \"github.com/coreos/rkt/store\"\r\n)\r\n\r\nfunc main() {\r\n        s, err := store.NewStore(\"/tmp/test/rkt\")\r\n        if err != nil {\r\n                fmt.Printf(\"cannot open store: %v\", err)\r\n                os.Exit(1)\r\n        }\r\n\r\n        runs := 10000\r\n        for i := 0; i < runs; i++ {\r\n                go func() {\r\n                        _, err = s.GetAllACIInfos(nil, false)\r\n                        if err != nil {\r\n                                fmt.Printf(\"cannot query db: %v\", err)\r\n                                os.Exit(1)\r\n                        }\r\n                }()\r\n        }\r\n\r\n        s.Close()\r\n}\r\n\r\n```\r\n\r\ncompile and execute the above command with something like:\r\n\r\n```\r\n\r\nwhile true; do\r\n        for i in $(seq 0 2); do\r\n                ./test01 &\r\n        done\r\n        wait\r\ndone\r\n```\r\n\r\nShould fix #2374 ","created_at":"2016-04-07T15:57:08Z","updated_at":"2016-04-07T19:45:17Z","closed_at":null,"merged_at":null,"merge_commit_sha":"4869c53091c5c751c814bda08153f12f22474e3b","assignee":null,"milestone":null,"commits_url":"https://api.github.com/repos/coreos/rkt/pulls/2391/commits","review_comments_url":"https://api.github.com/repos/coreos/rkt/pulls/2391/comments","review_comment_url":"https://api.github.com/repos/coreos/rkt/pulls/comments{/number}","comments_url":"https://api.github.com/repos/coreos/rkt/issues/2391/comments","statuses_url":"https://api.github.com/repos/coreos/rkt/statuses/78fc8d5ab8679d8793eef2d36d6956a10b686da8","head":{"label":"sgotti:fix_store_concurrency","ref":"fix_store_concurrency","sha":"78fc8d5ab8679d8793eef2d36d6956a10b686da8","user":{"login":"sgotti","id":1690273,"avatar_url":"https://avatars.githubusercontent.com/u/1690273?v=3","gravatar_id":"","url":"https://api.github.com/users/sgotti","html_url":"https://github.com/sgotti","followers_url":"https://api.github.com/users/sgotti/followers","following_url":"https://api.github.com/users/sgotti/following{/other_user}","gists_url":"https://api.github.com/users/sgotti/gists{/gist_id}","starred_url":"https://api.github.com/users/sgotti/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgotti/subscriptions","organizations_url":"https://api.github.com/users/sgotti/orgs","repos_url":"https://api.github.com/users/sgotti/repos","events_url":"https://api.github.com/users/sgotti/events{/privacy}","received_events_url":"https://api.github.com/users/sgotti/received_events","type":"User","site_admin":false},"repo":{"id":27428376,"name":"rkt","full_name":"sgotti/rkt","owner":{"login":"sgotti","id":1690273,"avatar_url":"https://avatars.githubusercontent.com/u/1690273?v=3","gravatar_id":"","url":"https://api.github.com/users/sgotti","html_url":"https://github.com/sgotti","followers_url":"https://api.github.com/users/sgotti/followers","following_url":"https://api.github.com/users/sgotti/following{/other_user}","gists_url":"https://api.github.com/users/sgotti/gists{/gist_id}","starred_url":"https://api.github.com/users/sgotti/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sgotti/subscriptions","organizations_url":"https://api.github.com/users/sgotti/orgs","repos_url":"https://api.github.com/users/sgotti/repos","events_url":"https://api.github.com/users/sgotti/events{/privacy}","received_events_url":"https://api.github.com/users/sgotti/received_events","type":"User","site_admin":false},"private":false,"html_url":"https://github.com/sgotti/rkt","description":"App Container runtime","fork":true,"url":"https://api.github.com/repos/sgotti/rkt","forks_url":"https://api.github.com/repos/sgotti/rkt/forks","keys_url":"https://api.github.com/repos/sgotti/rkt/keys{/key_id}","collaborators_url":"https://api.github.com/repos/sgotti/rkt/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/sgotti/rkt/teams","hooks_url":"https://api.github.com/repos/sgotti/rkt/hooks","issue_events_url":"https://api.github.com/repos/sgotti/rkt/issues/events{/number}","events_url":"https://api.github.com/repos/sgotti/rkt/events","assignees_url":"https://api.github.com/repos/sgotti/rkt/assignees{/user}","branches_url":"https://api.github.com/repos/sgotti/rkt/branches{/branch}","tags_url":"https://api.github.com/repos/sgotti/rkt/tags","blobs_url":"https://api.github.com/repos/sgotti/rkt/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/sgotti/rkt/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/sgotti/rkt/git/refs{/sha}","trees_url":"https://api.github.com/repos/sgotti/rkt/git/trees{/sha}","statuses_url":"https://api.github.com/repos/sgotti/rkt/statuses/{sha}","languages_url":"https://api.github.com/repos/sgotti/rkt/languages","stargazers_url":"https://api.github.com/repos/sgotti/rkt/stargazers","contributors_url":"https://api.github.com/repos/sgotti/rkt/contributors","subscribers_url":"https://api.github.com/repos/sgotti/rkt/subscribers","subscription_url":"https://api.github.com/repos/sgotti/rkt/subscription","commits_url":"https://api.github.com/repos/sgotti/rkt/commits{/sha}","git_commits_url":"https://api.github.com/repos/sgotti/rkt/git/commits{/sha}","comments_url":"https://api.github.com/repos/sgotti/rkt/comments{/number}","issue_comment_url":"https://api.github.com/repos/sgotti/rkt/issues/comments{/number}","contents_url":"https://api.github.com/repos/sgotti/rkt/contents/{+path}","compare_url":"https://api.github.com/repos/sgotti/rkt/compare/{base}...{head}","merges_url":"https://api.github.com/repos/sgotti/rkt/merges","archive_url":"https://api.github.com/repos/sgotti/rkt/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/sgotti/rkt/downloads","issues_url":"https://api.github.com/repos/sgotti/rkt/issues{/number}","pulls_url":"https://api.github.com/repos/sgotti/rkt/pulls{/number}","milestones_url":"https://api.github.com/repos/sgotti/rkt/milestones{/number}","notifications_url":"https://api.github.com/repos/sgotti/rkt/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/sgotti/rkt/labels{/name}","releases_url":"https://api.github.com/repos/sgotti/rkt/releases{/id}","deployments_url":"https://api.github.com/repos/sgotti/rkt/deployments","created_at":"2014-12-02T10:56:31Z","updated_at":"2016-03-21T15:26:18Z","pushed_at":"2016-04-07T15:56:05Z","git_url":"git://github.com/sgotti/rkt.git","ssh_url":"git@github.com:sgotti/rkt.git","clone_url":"https://github.com/sgotti/rkt.git","svn_url":"https://github.com/sgotti/rkt","homepage":"","size":19638,"stargazers_count":0,"watchers_count":0,"language":"Go","has_issues":false,"has_downloads":true,"has_wiki":true,"has_pages":false,"forks_count":0,"mirror_url":null,"open_issues_count":0,"forks":0,"open_issues":0,"watchers":0,"default_branch":"master"}},"base":{"label":"coreos:master","ref":"master","sha":"cfc0d7ff785b4ae38d8ed706f4ef84b30ab4d761","user":{"login":"coreos","id":3730757,"avatar_url":"https://avatars.githubusercontent.com/u/3730757?v=3","gravatar_id":"","url":"https://api.github.com/users/coreos","html_url":"https://github.com/coreos","followers_url":"https://api.github.com/users/coreos/followers","following_url":"https://api.github.com/users/coreos/following{/other_user}","gists_url":"https://api.github.com/users/coreos/gists{/gist_id}","starred_url":"https://api.github.com/users/coreos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coreos/subscriptions","organizations_url":"https://api.github.com/users/coreos/orgs","repos_url":"https://api.github.com/users/coreos/repos","events_url":"https://api.github.com/users/coreos/events{/privacy}","received_events_url":"https://api.github.com/users/coreos/received_events","type":"Organization","site_admin":false},"repo":{"id":26509369,"name":"rkt","full_name":"coreos/rkt","owner":{"login":"coreos","id":3730757,"avatar_url":"https://avatars.githubusercontent.com/u/3730757?v=3","gravatar_id":"","url":"https://api.github.com/users/coreos","html_url":"https://github.com/coreos","followers_url":"https://api.github.com/users/coreos/followers","following_url":"https://api.github.com/users/coreos/following{/other_user}","gists_url":"https://api.github.com/users/coreos/gists{/gist_id}","starred_url":"https://api.github.com/users/coreos/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/coreos/subscriptions","organizations_url":"https://api.github.com/users/coreos/orgs","repos_url":"https://api.github.com/users/coreos/repos","events_url":"https://api.github.com/users/coreos/events{/privacy}","received_events_url":"https://api.github.com/users/coreos/received_events","type":"Organization","site_admin":false},"private":false,"html_url":"https://github.com/coreos/rkt","description":"rkt is an App Container runtime for Linux","fork":false,"url":"https://api.github.com/repos/coreos/rkt","forks_url":"https://api.github.com/repos/coreos/rkt/forks","keys_url":"https://api.github.com/repos/coreos/rkt/keys{/key_id}","collaborators_url":"https://api.github.com/repos/coreos/rkt/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/coreos/rkt/teams","hooks_url":"https://api.github.com/repos/coreos/rkt/hooks","issue_events_url":"https://api.github.com/repos/coreos/rkt/issues/events{/number}","events_url":"https://api.github.com/repos/coreos/rkt/events","assignees_url":"https://api.github.com/repos/coreos/rkt/assignees{/user}","branches_url":"https://api.github.com/repos/coreos/rkt/branches{/branch}","tags_url":"https://api.github.com/repos/coreos/rkt/tags","blobs_url":"https://api.github.com/repos/coreos/rkt/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/coreos/rkt/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/coreos/rkt/git/refs{/sha}","trees_url":"https://api.github.com/repos/coreos/rkt/git/trees{/sha}","statuses_url":"https://api.github.com/repos/coreos/rkt/statuses/{sha}","languages_url":"https://api.github.com/repos/coreos/rkt/languages","stargazers_url":"https://api.github.com/repos/coreos/rkt/stargazers","contributors_url":"https://api.github.com/repos/coreos/rkt/contributors","subscribers_url":"https://api.github.com/repos/coreos/rkt/subscribers","subscription_url":"https://api.github.com/repos/coreos/rkt/subscription","commits_url":"https://api.github.com/repos/coreos/rkt/commits{/sha}","git_commits_url":"https://api.github.com/repos/coreos/rkt/git/commits{/sha}","comments_url":"https://api.github.com/repos/coreos/rkt/comments{/number}","issue_comment_url":"https://api.github.com/repos/coreos/rkt/issues/comments{/number}","contents_url":"https://api.github.com/repos/coreos/rkt/contents/{+path}","compare_url":"https://api.github.com/repos/coreos/rkt/compare/{base}...{head}","merges_url":"https://api.github.com/repos/coreos/rkt/merges","archive_url":"https://api.github.com/repos/coreos/rkt/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/coreos/rkt/downloads","issues_url":"https://api.github.com/repos/coreos/rkt/issues{/number}","pulls_url":"https://api.github.com/repos/coreos/rkt/pulls{/number}","milestones_url":"https://api.github.com/repos/coreos/rkt/milestones{/number}","notifications_url":"https://api.github.com/repos/coreos/rkt/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/coreos/rkt/labels{/name}","releases_url":"https://api.github.com/repos/coreos/rkt/releases{/id}","deployments_url":"https://api.github.com/repos/coreos/rkt/deployments","created_at":"2014-11-11T23:13:18Z","updated_at":"2016-04-07T16:39:32Z","pushed_at":"2016-04-07T16:53:51Z","git_url":"git://github.com/coreos/rkt.git","ssh_url":"git@github.com:coreos/rkt.git","clone_url":"https://github.com/coreos/rkt.git","svn_url":"https://github.com/coreos/rkt","homepage":"","size":26059,"stargazers_count":5377,"watchers_count":5377,"language":"Go","has_issues":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":491,"mirror_url":null,"open_issues_count":275,"forks":491,"open_issues":275,"watchers":5377,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/coreos/rkt/pulls/2391"},"html":{"href":"https://github.com/coreos/rkt/pull/2391"},"issue":{"href":"https://api.github.com/repos/coreos/rkt/issues/2391"},"comments":{"href":"https://api.github.com/repos/coreos/rkt/issues/2391/comments"},"review_comments":{"href":"https://api.github.com/repos/coreos/rkt/pulls/2391/comments"},"review_comment":{"href":"https://api.github.com/repos/coreos/rkt/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/coreos/rkt/pulls/2391/commits"},"statuses":{"href":"https://api.github.com/repos/coreos/rkt/statuses/78fc8d5ab8679d8793eef2d36d6956a10b686da8"}}}}