{"action":"created","issue":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7504","repository_url":"https://api.github.com/repos/cockroachdb/cockroach","labels_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7504/labels{/name}","comments_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7504/comments","events_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7504/events","html_url":"https://github.com/cockroachdb/cockroach/pull/7504","id":162715211,"number":7504,"title":"sql: avoid deadlock in lease acquisition","user":{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":13,"created_at":"2016-06-28T15:29:07Z","updated_at":"2016-06-29T20:28:43Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/cockroachdb/cockroach/pulls/7504","html_url":"https://github.com/cockroachdb/cockroach/pull/7504","diff_url":"https://github.com/cockroachdb/cockroach/pull/7504.diff","patch_url":"https://github.com/cockroachdb/cockroach/pull/7504.patch"},"body":"We were locking leaseState, tableNameCache in Release(), but\r\ntableNameCache,LeaseState in AcquireByName.\r\nRefactored Release() to not require both locks at the same time.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"35\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/cockroachdb/cockroach/7504)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/cockroachdb/cockroach/issues/comments/229478163","html_url":"https://github.com/cockroachdb/cockroach/pull/7504#issuecomment-229478163","issue_url":"https://api.github.com/repos/cockroachdb/cockroach/issues/7504","id":229478163,"user":{"login":"andreimatei","id":377201,"avatar_url":"https://avatars.githubusercontent.com/u/377201?v=3","gravatar_id":"","url":"https://api.github.com/users/andreimatei","html_url":"https://github.com/andreimatei","followers_url":"https://api.github.com/users/andreimatei/followers","following_url":"https://api.github.com/users/andreimatei/following{/other_user}","gists_url":"https://api.github.com/users/andreimatei/gists{/gist_id}","starred_url":"https://api.github.com/users/andreimatei/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/andreimatei/subscriptions","organizations_url":"https://api.github.com/users/andreimatei/orgs","repos_url":"https://api.github.com/users/andreimatei/repos","events_url":"https://api.github.com/users/andreimatei/events{/privacy}","received_events_url":"https://api.github.com/users/andreimatei/received_events","type":"User","site_admin":false},"created_at":"2016-06-29T20:28:43Z","updated_at":"2016-06-29T20:28:43Z","body":"\n\n\n\nReview status: all files reviewed at latest revision, 23 unresolved discussions, some commit checks failed.\n\n---\n\n*[server/server.go, line 160 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRzilKZEHpA2naHGj5:-KLSE4wRaX4kUNjUONfM:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/23dc7005de65e0711283377ae7a857cc712fc759/server/server.go#L160)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> if you're going to make this change, do it everywhere. there are still references to `stopper` above.\n</details>\nDone.\n\n---\n\n*[sql/lease.go, line 640 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLMm0vAeJXwIeH6smT0:-KLSL3y9bXHrS9qh5hQw:1601231304) ([raw file](https://github.com/cockroachdb/cockroach/blob/c80d81782d3618a3e236c5bbda1a9857e75465a1/sql/lease.go#L640)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> This still allocates. Make it a method? It'll mirror incRefCount nicely.\n</details>\nit doesn't mirror `incRefCount` because this interacts with the `tableState` (looks at the `deleted` bit).\nWhat allocates exactly?\n\n---\n\n*[sql/lease.go, line 656 \\[r1\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLMnos0d0V1JBTGRNOt:-KLSNxtzZgWcakoK26Z_:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/c80d81782d3618a3e236c5bbda1a9857e75465a1/sql/lease.go#L656)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> There are multiple blocks here, though. I had a hard time understanding this, especially because the first condition mentioned in the comment is not the first condition checked in the code!\n</details>\nDone.\n\n---\n\n*[sql/lease.go, line 739 \\[r2\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRvTmckM03d3_yaZXQ:-KLSjCKsdFRk4BHAXiCn:166874) ([raw file](https://github.com/cockroachdb/cockroach/blob/558ef5bb8b44e079d5a16a31abf1ad8d572c9ddd/sql/lease.go#L739)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/Check out/See/\n</details>\n?\n\n---\n\n*[sql/lease.go, line 847 \\[r4\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRxKin4DNJcV2wH0ba:-KLSjUpw4rf1_hLvDXFK:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/382c4f1be26841ab9872c19532e867268dd649a4/sql/lease.go#L847)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/tables/table/\n> \n> how about you make this method return an error? it's not obvious to me that this behaviour is correct for all callers.\n</details>\nDone.\n\n---\n\n*[sql/lease.go, line 96 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRxZYa5x62io45xOnC:-KLSKpbjL1A7CqCxW88f:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease.go#L96)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> remove the defer\n</details>\nDone.\n\n---\n\n*[sql/lease.go, line 99 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLSfY1eIJ8ibszq6oHY:-KLSpF8P52AiggvxBB84:-1790489899) ([raw file](https://github.com/cockroachdb/cockroach/blob/23dc7005de65e0711283377ae7a857cc712fc759/sql/lease.go#L99)):*\n<details><summary><i>Previously, vivekmenezes wrote…</i></summary>\n> should we be checking s.released every time we increment the refcount? We increment in two places, we check released only in one place. Just wondering if we can make the increment more generic with the increment and check with released all in one? \n</details>\nthe check of `released` is only needed when we return the lease from the `tableNameCache`, as opposed to from a `tableState`. The point of `released` is to prevent races between the lease releases (which are always done through a `tableState` and this name cache structure that's not updated atomically with the release).\n\n---\n\n*[sql/lease.go, line 694 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRzsjRhll8wjYXou-5:-KLSOF19lin0SCKh0r-j:760905280) ([raw file](https://github.com/cockroachdb/cockroach/blob/23dc7005de65e0711283377ae7a857cc712fc759/sql/lease.go#L694)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> can you log it instead? seems to me that there's no downside. In fact, I'd use the same \"Error releasing lease %q: %s\" as below (though Error shouldn't be capitalized).\n</details>\nWell the only error the stopper can return is when it's stopping. And in that case the error is expected, I don't see a point in communicating it to anybody.\nOne thing I've considered doing here on error was to retry the release inline in case of error, since we'd ideally like to do some best effort to release leases after a server started draining. But it didn't seem to be worth the effort.\n\n---\n\n*[sql/lease.go, line 912 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLS-RX-lQEUXAdmDmWy:-KLSRrdwe0cvkENNE0ph:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/23dc7005de65e0711283377ae7a857cc712fc759/sql/lease.go#L912)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> i don't think you should do this - tests should create the stopper and `defer stopper.Stop()` it. There is only a handful of tests to update.\n</details>\nDone.\n\n---\n\n*[sql/lease.go, line 1083 \\[r6\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLS-GgZ_CSgcKHLtmiJ:-KLSVDqDaBO0dkygKqQD:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/23dc7005de65e0711283377ae7a857cc712fc759/sql/lease.go#L1083)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> why do you need to pass the stopper? you already have it from the LeaseManager.\n</details>\nDone.\n\n---\n\n*[sql/lease_internal_test.go, line 341 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRyTNxKsG3uLK_D-NP:-KLSjm5-tmlx_rOXsjyR:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease_internal_test.go#L341)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/has/had/\n</details>\nDone.\n\n---\n\n*[sql/lease_internal_test.go, line 369 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRybFmzBgsFLhgki0Y:-KLSjyHNCLvJJ-z0kzyD:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease_internal_test.go#L369)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> how about just\n> ```\n> _, err := leaseManager.AcquireByName(txn, tableDesc.ParentID, \"test\")\n> return err\n> ```\n</details>\nDone.\n\n---\n\n*[sql/lease_internal_test.go, line 408 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRzVQ7I9On26VFmbo6:-KLSkF7Txa1UhZxuDnho:136961239) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease_internal_test.go#L408)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> why do you panic here and below but return above?\n</details>\nswitched to panic above\n\n---\n\n*[sql/lease_internal_test.go, line 425 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRynrzmb3fiDXGLcnW:-KLSkojeNK4tctpbw1Kz:-143166320) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease_internal_test.go#L425)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this tingles my spidey-sense: it looks like it races with the wg.Done() in the closure above. Seems like you don't need the wait group at all, just do one of these things in the main goroutine.\n</details>\nI think `Add` and `Done` can be called in any order even if the counter reaches 0 before going up again; it's just `Wait` that needs to be called after all the positive adds. In any case, the first goroutine cannot finish before the second one starts, `leaseChan` acts as a semaphore. Added a comment about that.\n\nBut you're right that I can do the second goroutine's code in the main one. Removed. But the waitgroup has to stay for the first one.\n\n---\n\n*[sql/lease_internal_test.go, line 442 \\[r5\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRzJqyk-wj5dvePshC:-KLSlvj9HUiMCvDvJf2i:-534127091) ([raw file](https://github.com/cockroachdb/cockroach/blob/9cda7dfce5de1685e7c9dd080d0b7ba43f6559d1/sql/lease_internal_test.go#L442)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> nit return this error (you'll panic in the caller anyway, I suspect).\n</details>\nmy philosophy is to generally panic early in tests. Why rely on some particular behavior from the caller?\n\n---\n\n*[sql/testutils.go, line 17 \\[r2\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRvr4geGDtzgJOWmk9:-KLSVm4uRa7iX5eYqGaT:240957905) ([raw file](https://github.com/cockroachdb/cockroach/blob/558ef5bb8b44e079d5a16a31abf1ad8d572c9ddd/sql/testutils.go#L17)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> this file should end in _test.go - we don't want this linked into production code.\n</details>\nI prefer these `testutils.go` files, even though they get linked, because that's a clear place to look for crap like this. If they stay in some random `_test.go` files, there's much less chance they're gonna be reused. There's a `testutils.go` file in `sqlbase` too and it has worked well.\nWhere the utils don't need to be in a specific package, I create a `testutils` subpackage for them (or put stuff in the toplevel `testutils` package, although I don't prefer that), but that's unfortunately not the case here.\n\n---\n\n*[sql/testutils.go, line 35 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRwBOIwrf_6a6lG5qV:-KLSXMEB1AN92N4NhiXE:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L35)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> broken sentence\n</details>\nDone.\n\n---\n\n*[sql/testutils.go, line 36 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRwD9f3kldWP16BnRb:-KLSXMTlpxGlMpCckO3J:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L36)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> s/published/sent/\n</details>\nDone.\n\n---\n\n*[sql/testutils.go, line 37 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRwZbbKAc8IgMCck1L:-KLSX30hRnVkXIvJYNBz:160408385) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L37)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> why don't\n</details>\nha?\n\n---\n\n*[sql/testutils.go, line 54 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRw9tl0lrtUD-tgngQ:-KLSXPMAx_uWRkNJ4KZ-:-499146009) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L54)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> broken sentence\n</details>\nDone.\n\n---\n\n*[sql/testutils.go, line 62 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRwc3Y_Jnnu3xvWNgm:-KLSh7uL5ZATwRLzjcNw:-16423376) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L62)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> you could lift this restriction by making `waitingFor` a\n> ```\n> map[*LeaseState]struct{\n>   cond sync.Cond\n>   err error\n> }\n> ```\n</details>\nwell supporting more than one waiter is trickier because the waiters would need to coordinate the removal of the wait slot from the map\n\n---\n\n*[sql/testutils.go, line 72 \\[r3\\]](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504#-KLRwtInMkJe6M7MKxrk:-KLSayv9-vRVLEU9cW4o:-854276960) ([raw file](https://github.com/cockroachdb/cockroach/blob/40adfd088a44bb17f9001408e7bf1a0d4c8c463a/sql/testutils.go#L72)):*\n<details><summary><i>Previously, tamird (Tamir Duberstein) wrote…</i></summary>\n> no need to wait until after the channel receive for this. also, because this waits until after the receive, it means that the panic above doesn't trigger correctly.\n</details>\nthe `delete` must happen after the channel has been signaled, otherwise `LeaseReleasedNotification` won't find the channel in the map...\nNot sure what you mean about the panic not triggering correctly; did you mean it's incorrect if 2 guys try to wait for the same lease? Cause that's not supported.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/cockroachdb/cockroach/7504)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"andreimatei"}}