{"action":"created","issue":{"url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/85","repository_url":"https://api.github.com/repos/pivotalsoftware/quickstep","labels_url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/85/labels{/name}","comments_url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/85/comments","events_url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/85/events","html_url":"https://github.com/pivotalsoftware/quickstep/pull/85","id":140784017,"number":85,"title":"Storage manager concurrency bug","user":{"login":"cramja","id":5708822,"avatar_url":"https://avatars.githubusercontent.com/u/5708822?v=3","gravatar_id":"","url":"https://api.github.com/users/cramja","html_url":"https://github.com/cramja","followers_url":"https://api.github.com/users/cramja/followers","following_url":"https://api.github.com/users/cramja/following{/other_user}","gists_url":"https://api.github.com/users/cramja/gists{/gist_id}","starred_url":"https://api.github.com/users/cramja/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cramja/subscriptions","organizations_url":"https://api.github.com/users/cramja/orgs","repos_url":"https://api.github.com/users/cramja/repos","events_url":"https://api.github.com/users/cramja/events{/privacy}","received_events_url":"https://api.github.com/users/cramja/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":15,"created_at":"2016-03-14T20:08:30Z","updated_at":"2016-03-22T19:19:57Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/pivotalsoftware/quickstep/pulls/85","html_url":"https://github.com/pivotalsoftware/quickstep/pull/85","diff_url":"https://github.com/pivotalsoftware/quickstep/pull/85.diff","patch_url":"https://github.com/pivotalsoftware/quickstep/pull/85.patch"},"body":"Fixes the deadlock-causing bug in the storage manager. Concludes #70 .\r\n\r\nThe essence of the bug was that threads acquired 2 locks during an eviction phase in the method `makeRoomForBlock`. Depending on the interleaving, thread A could request lock 1 while holding lock 2 and thread B could hold lock 2 while requesting 1, classic deadlock.\r\n\r\nThe change simply moves the call to makeRoomForBlock to a section of calling code where no locks already held, removing the possibility of deadlock. \r\n\r\nThis move does not change the correctness of the StorageManager. Though it is possible to get an interleaving which will result in temporarily over-shooting the size limit of the StorageManager, this was possible in the previous code as well. It does not present a serious issue.\r\n\r\n**Note:** \r\nOn rare occasions (one out of 50) when I ran the ctest suite on the quickstep box, I got a RefCount bug\r\n\r\n```\r\nquickstep_queryoptimizer_tests_ExecutionGeneratorTest: /home/spehlmann/qs/storage/EvictionPolicy.cpp:411:\r\nvoid quickstep::LRUKEvictionPolicyImpl<K>::blockEvicted(quickstep::block_id) \r\n[with long unsigned int K = 2ul; quickstep::block_id = long unsigned int]: \r\nAssertion `blocks_[block].ref_count_ == 0' failed.\r\n```\r\n\r\nWhich I do not think is related to this patch."},"comment":{"url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/comments/199972488","html_url":"https://github.com/pivotalsoftware/quickstep/pull/85#issuecomment-199972488","issue_url":"https://api.github.com/repos/pivotalsoftware/quickstep/issues/85","id":199972488,"user":{"login":"cramja","id":5708822,"avatar_url":"https://avatars.githubusercontent.com/u/5708822?v=3","gravatar_id":"","url":"https://api.github.com/users/cramja","html_url":"https://github.com/cramja","followers_url":"https://api.github.com/users/cramja/followers","following_url":"https://api.github.com/users/cramja/following{/other_user}","gists_url":"https://api.github.com/users/cramja/gists{/gist_id}","starred_url":"https://api.github.com/users/cramja/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/cramja/subscriptions","organizations_url":"https://api.github.com/users/cramja/orgs","repos_url":"https://api.github.com/users/cramja/repos","events_url":"https://api.github.com/users/cramja/events{/privacy}","received_events_url":"https://api.github.com/users/cramja/received_events","type":"User","site_admin":false},"created_at":"2016-03-22T19:19:57Z","updated_at":"2016-03-22T19:19:57Z","body":"To avoid further merges with master and because we have agreed that it's good to go, I will merge this PR once Travis returns."}}