payload: {"action":"created","comment":{"url":"https://api.github.com/repos/Yelp/paasta/pulls/comments/321381299","pull_request_review_id":284412023,"id":321381299,"node_id":"MDI0OlB1bGxSZXF1ZXN0UmV2aWV3Q29tbWVudDMyMTM4MTI5OQ==","diff_hunk":"@@ -10,30 +10,41 @@\n from typing import DefaultDict\n from typing import Dict\n from typing import List\n+from typing import Optional\n from typing import TypeVar\n \n \n T = TypeVar(\"T\")\n \n \n def async_ttl_cache(\n-    ttl: float = 300, cleanup_self: bool = False\n+    ttl: Optional[float] = 300, cleanup_self: bool = False\n ) -> Callable[\n     [Callable[..., Awaitable[T]]], Callable[..., Awaitable[T]]  # wrapped  # inner\n ]:\n     async def call_or_get_from_cache(cache, async_func, args, kwargs):\n         key = functools._make_key(args, kwargs, typed=False)\n         try:\n             future, last_update = cache[key]\n-            if ttl > 0 and time.time() - last_update > ttl:\n+            if ttl is not None and time.time() - last_update > ttl:\n                 raise KeyError\n         except KeyError:\n             future = asyncio.ensure_future(async_func(*args, **kwargs))\n             # set the timestamp to +infinity so that we always wait on the in-flight request.\n             cache[key] = (future, float(\"Inf\"))\n-        value = await future\n-        cache[key] = (future, time.time())\n-        return value\n+\n+        try:\n+            value = await future\n+        except Exception:\n+            # only update the cache if it's the same future we awaited and\n+            # it hasn't already been updated by another coroutine\n+            if cache[key] == (future, float(\"Inf\")):\n+                del cache[key]\n+            raise\n+        else:\n+            if cache[key] == (future, float(\"Inf\")):","path":"paasta_tools/async_utils.py","position":41,"original_position":41,"commit_id":"5931f29c70f8392071513338efe44dc7bc58969d","original_commit_id":"5931f29c70f8392071513338efe44dc7bc58969d","user":{"login":"stug","id":3210321,"node_id":"MDQ6VXNlcjMyMTAzMjE=","avatar_url":"https://avatars2.githubusercontent.com/u/3210321?v=4","gravatar_id":"","url":"https://api.github.com/users/stug","html_url":"https://github.com/stug","followers_url":"https://api.github.com/users/stug/followers","following_url":"https://api.github.com/users/stug/following{/other_user}","gists_url":"https://api.github.com/users/stug/gists{/gist_id}","starred_url":"https://api.github.com/users/stug/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stug/subscriptions","organizations_url":"https://api.github.com/users/stug/orgs","repos_url":"https://api.github.com/users/stug/repos","events_url":"https://api.github.com/users/stug/events{/privacy}","received_events_url":"https://api.github.com/users/stug/received_events","type":"User","site_admin":false},"body":"Well so when we pull a future out of the cache, if that future has already returned then another coroutine should have already set the timestamp.  If we were to re-set it then we would effectively increase the ttl, which I don't think we want.\r\n\r\nIf the future we get from the cache has _not_ returned, then the timestamp on the cache entry should still be `float(\"Inf\")`, in which case the first coroutine to return from awaiting the future will update the timestamp to `time.time()`, which starts the ttl count down.''\r\n\r\nFor the case where the future raises an Exception, I think you are right that we could get away with only looking at the future, but I figured this keeps the logic more consistent.","created_at":"2019-09-05T17:10:43Z","updated_at":"2019-09-05T17:10:44Z","html_url":"https://github.com/Yelp/paasta/pull/2403#discussion_r321381299","pull_request_url":"https://api.github.com/repos/Yelp/paasta/pulls/2403","author_association":"CONTRIBUTOR","_links":{"self":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/comments/321381299"},"html":{"href":"https://github.com/Yelp/paasta/pull/2403#discussion_r321381299"},"pull_request":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/2403"}},"in_reply_to_id":321104953},"pull_request":{"url":"https://api.github.com/repos/Yelp/paasta/pulls/2403","id":314289120,"node_id":"MDExOlB1bGxSZXF1ZXN0MzE0Mjg5MTIw","html_url":"https://github.com/Yelp/paasta/pull/2403","diff_url":"https://github.com/Yelp/paasta/pull/2403.diff","patch_url":"https://github.com/Yelp/paasta/pull/2403.patch","issue_url":"https://api.github.com/repos/Yelp/paasta/issues/2403","number":2403,"state":"open","locked":false,"title":"Stop async_ttl_cache from caching failures","user":{"login":"stug","id":3210321,"node_id":"MDQ6VXNlcjMyMTAzMjE=","avatar_url":"https://avatars2.githubusercontent.com/u/3210321?v=4","gravatar_id":"","url":"https://api.github.com/users/stug","html_url":"https://github.com/stug","followers_url":"https://api.github.com/users/stug/followers","following_url":"https://api.github.com/users/stug/following{/other_user}","gists_url":"https://api.github.com/users/stug/gists{/gist_id}","starred_url":"https://api.github.com/users/stug/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stug/subscriptions","organizations_url":"https://api.github.com/users/stug/orgs","repos_url":"https://api.github.com/users/stug/repos","events_url":"https://api.github.com/users/stug/events{/privacy}","received_events_url":"https://api.github.com/users/stug/received_events","type":"User","site_admin":false},"body":"Currently if the future raises an exception, we store that in the cache, and we never end up executing the logic that changes its `last_update` timestamp back to something other than `float(\"Inf\")`.  This makes sure that we remove the cache entry on exception.\r\n\r\nThis also handles a subtle race condition where two coroutines get the same in-flight future out of the cache, and one of them returns long before the other.  The first to return will either set the `last_update` timestamp on the entry so that it eventually expires, or, if the future raised an exception, it will delete the cache entry.  It's possible that, before the second coroutine gets to run again, a third coroutine will run the cached method and insert a new in-flight coroutine into the cache.  If the second coroutine gets control again, it shouldn't alter the cache since its context is long out of date.  I am pretty sure my code fixes this, but I can't for the life of me figure out how to test that case very easily.  The test to make sure we share in-flight requests involves pretty complex asyncio synchronization primitives as it is :-/","created_at":"2019-09-05T01:23:47Z","updated_at":"2019-09-05T17:10:44Z","closed_at":null,"merged_at":null,"merge_commit_sha":"b1f45616793975158b2d843caf56efbe4affd9d2","assignee":{"login":"stug","id":3210321,"node_id":"MDQ6VXNlcjMyMTAzMjE=","avatar_url":"https://avatars2.githubusercontent.com/u/3210321?v=4","gravatar_id":"","url":"https://api.github.com/users/stug","html_url":"https://github.com/stug","followers_url":"https://api.github.com/users/stug/followers","following_url":"https://api.github.com/users/stug/following{/other_user}","gists_url":"https://api.github.com/users/stug/gists{/gist_id}","starred_url":"https://api.github.com/users/stug/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stug/subscriptions","organizations_url":"https://api.github.com/users/stug/orgs","repos_url":"https://api.github.com/users/stug/repos","events_url":"https://api.github.com/users/stug/events{/privacy}","received_events_url":"https://api.github.com/users/stug/received_events","type":"User","site_admin":false},"assignees":[{"login":"stug","id":3210321,"node_id":"MDQ6VXNlcjMyMTAzMjE=","avatar_url":"https://avatars2.githubusercontent.com/u/3210321?v=4","gravatar_id":"","url":"https://api.github.com/users/stug","html_url":"https://github.com/stug","followers_url":"https://api.github.com/users/stug/followers","following_url":"https://api.github.com/users/stug/following{/other_user}","gists_url":"https://api.github.com/users/stug/gists{/gist_id}","starred_url":"https://api.github.com/users/stug/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/stug/subscriptions","organizations_url":"https://api.github.com/users/stug/orgs","repos_url":"https://api.github.com/users/stug/repos","events_url":"https://api.github.com/users/stug/events{/privacy}","received_events_url":"https://api.github.com/users/stug/received_events","type":"User","site_admin":false}],"requested_reviewers":[{"login":"EvanKrall","id":233961,"node_id":"MDQ6VXNlcjIzMzk2MQ==","avatar_url":"https://avatars2.githubusercontent.com/u/233961?v=4","gravatar_id":"","url":"https://api.github.com/users/EvanKrall","html_url":"https://github.com/EvanKrall","followers_url":"https://api.github.com/users/EvanKrall/followers","following_url":"https://api.github.com/users/EvanKrall/following{/other_user}","gists_url":"https://api.github.com/users/EvanKrall/gists{/gist_id}","starred_url":"https://api.github.com/users/EvanKrall/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/EvanKrall/subscriptions","organizations_url":"https://api.github.com/users/EvanKrall/orgs","repos_url":"https://api.github.com/users/EvanKrall/repos","events_url":"https://api.github.com/users/EvanKrall/events{/privacy}","received_events_url":"https://api.github.com/users/EvanKrall/received_events","type":"User","site_admin":false}],"requested_teams":[],"labels":[],"milestone":null,"commits_url":"https://api.github.com/repos/Yelp/paasta/pulls/2403/commits","review_comments_url":"https://api.github.com/repos/Yelp/paasta/pulls/2403/comments","review_comment_url":"https://api.github.com/repos/Yelp/paasta/pulls/comments{/number}","comments_url":"https://api.github.com/repos/Yelp/paasta/issues/2403/comments","statuses_url":"https://api.github.com/repos/Yelp/paasta/statuses/5931f29c70f8392071513338efe44dc7bc58969d","head":{"label":"Yelp:fix_async_ttl_cache","ref":"fix_async_ttl_cache","sha":"5931f29c70f8392071513338efe44dc7bc58969d","user":{"login":"Yelp","id":49071,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQ5MDcx","avatar_url":"https://avatars1.githubusercontent.com/u/49071?v=4","gravatar_id":"","url":"https://api.github.com/users/Yelp","html_url":"https://github.com/Yelp","followers_url":"https://api.github.com/users/Yelp/followers","following_url":"https://api.github.com/users/Yelp/following{/other_user}","gists_url":"https://api.github.com/users/Yelp/gists{/gist_id}","starred_url":"https://api.github.com/users/Yelp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Yelp/subscriptions","organizations_url":"https://api.github.com/users/Yelp/orgs","repos_url":"https://api.github.com/users/Yelp/repos","events_url":"https://api.github.com/users/Yelp/events{/privacy}","received_events_url":"https://api.github.com/users/Yelp/received_events","type":"Organization","site_admin":false},"repo":{"id":44998824,"node_id":"MDEwOlJlcG9zaXRvcnk0NDk5ODgyNA==","name":"paasta","full_name":"Yelp/paasta","private":false,"owner":{"login":"Yelp","id":49071,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQ5MDcx","avatar_url":"https://avatars1.githubusercontent.com/u/49071?v=4","gravatar_id":"","url":"https://api.github.com/users/Yelp","html_url":"https://github.com/Yelp","followers_url":"https://api.github.com/users/Yelp/followers","following_url":"https://api.github.com/users/Yelp/following{/other_user}","gists_url":"https://api.github.com/users/Yelp/gists{/gist_id}","starred_url":"https://api.github.com/users/Yelp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Yelp/subscriptions","organizations_url":"https://api.github.com/users/Yelp/orgs","repos_url":"https://api.github.com/users/Yelp/repos","events_url":"https://api.github.com/users/Yelp/events{/privacy}","received_events_url":"https://api.github.com/users/Yelp/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/Yelp/paasta","description":"An open, distributed platform as a service","fork":false,"url":"https://api.github.com/repos/Yelp/paasta","forks_url":"https://api.github.com/repos/Yelp/paasta/forks","keys_url":"https://api.github.com/repos/Yelp/paasta/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Yelp/paasta/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Yelp/paasta/teams","hooks_url":"https://api.github.com/repos/Yelp/paasta/hooks","issue_events_url":"https://api.github.com/repos/Yelp/paasta/issues/events{/number}","events_url":"https://api.github.com/repos/Yelp/paasta/events","assignees_url":"https://api.github.com/repos/Yelp/paasta/assignees{/user}","branches_url":"https://api.github.com/repos/Yelp/paasta/branches{/branch}","tags_url":"https://api.github.com/repos/Yelp/paasta/tags","blobs_url":"https://api.github.com/repos/Yelp/paasta/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Yelp/paasta/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Yelp/paasta/git/refs{/sha}","trees_url":"https://api.github.com/repos/Yelp/paasta/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Yelp/paasta/statuses/{sha}","languages_url":"https://api.github.com/repos/Yelp/paasta/languages","stargazers_url":"https://api.github.com/repos/Yelp/paasta/stargazers","contributors_url":"https://api.github.com/repos/Yelp/paasta/contributors","subscribers_url":"https://api.github.com/repos/Yelp/paasta/subscribers","subscription_url":"https://api.github.com/repos/Yelp/paasta/subscription","commits_url":"https://api.github.com/repos/Yelp/paasta/commits{/sha}","git_commits_url":"https://api.github.com/repos/Yelp/paasta/git/commits{/sha}","comments_url":"https://api.github.com/repos/Yelp/paasta/comments{/number}","issue_comment_url":"https://api.github.com/repos/Yelp/paasta/issues/comments{/number}","contents_url":"https://api.github.com/repos/Yelp/paasta/contents/{+path}","compare_url":"https://api.github.com/repos/Yelp/paasta/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Yelp/paasta/merges","archive_url":"https://api.github.com/repos/Yelp/paasta/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Yelp/paasta/downloads","issues_url":"https://api.github.com/repos/Yelp/paasta/issues{/number}","pulls_url":"https://api.github.com/repos/Yelp/paasta/pulls{/number}","milestones_url":"https://api.github.com/repos/Yelp/paasta/milestones{/number}","notifications_url":"https://api.github.com/repos/Yelp/paasta/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Yelp/paasta/labels{/name}","releases_url":"https://api.github.com/repos/Yelp/paasta/releases{/id}","deployments_url":"https://api.github.com/repos/Yelp/paasta/deployments","created_at":"2015-10-26T21:35:53Z","updated_at":"2019-09-04T21:28:48Z","pushed_at":"2019-09-05T14:04:26Z","git_url":"git://github.com/Yelp/paasta.git","ssh_url":"git@github.com:Yelp/paasta.git","clone_url":"https://github.com/Yelp/paasta.git","svn_url":"https://github.com/Yelp/paasta","homepage":"","size":64704,"stargazers_count":1364,"watchers_count":1364,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":169,"mirror_url":null,"archived":false,"disabled":false,"open_issues_count":96,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0","node_id":"MDc6TGljZW5zZTI="},"forks":169,"open_issues":96,"watchers":1364,"default_branch":"master"}},"base":{"label":"Yelp:master","ref":"master","sha":"7ac8f6443a1377590b8e714a9e7791b22ba4d8ca","user":{"login":"Yelp","id":49071,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQ5MDcx","avatar_url":"https://avatars1.githubusercontent.com/u/49071?v=4","gravatar_id":"","url":"https://api.github.com/users/Yelp","html_url":"https://github.com/Yelp","followers_url":"https://api.github.com/users/Yelp/followers","following_url":"https://api.github.com/users/Yelp/following{/other_user}","gists_url":"https://api.github.com/users/Yelp/gists{/gist_id}","starred_url":"https://api.github.com/users/Yelp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Yelp/subscriptions","organizations_url":"https://api.github.com/users/Yelp/orgs","repos_url":"https://api.github.com/users/Yelp/repos","events_url":"https://api.github.com/users/Yelp/events{/privacy}","received_events_url":"https://api.github.com/users/Yelp/received_events","type":"Organization","site_admin":false},"repo":{"id":44998824,"node_id":"MDEwOlJlcG9zaXRvcnk0NDk5ODgyNA==","name":"paasta","full_name":"Yelp/paasta","private":false,"owner":{"login":"Yelp","id":49071,"node_id":"MDEyOk9yZ2FuaXphdGlvbjQ5MDcx","avatar_url":"https://avatars1.githubusercontent.com/u/49071?v=4","gravatar_id":"","url":"https://api.github.com/users/Yelp","html_url":"https://github.com/Yelp","followers_url":"https://api.github.com/users/Yelp/followers","following_url":"https://api.github.com/users/Yelp/following{/other_user}","gists_url":"https://api.github.com/users/Yelp/gists{/gist_id}","starred_url":"https://api.github.com/users/Yelp/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Yelp/subscriptions","organizations_url":"https://api.github.com/users/Yelp/orgs","repos_url":"https://api.github.com/users/Yelp/repos","events_url":"https://api.github.com/users/Yelp/events{/privacy}","received_events_url":"https://api.github.com/users/Yelp/received_events","type":"Organization","site_admin":false},"html_url":"https://github.com/Yelp/paasta","description":"An open, distributed platform as a service","fork":false,"url":"https://api.github.com/repos/Yelp/paasta","forks_url":"https://api.github.com/repos/Yelp/paasta/forks","keys_url":"https://api.github.com/repos/Yelp/paasta/keys{/key_id}","collaborators_url":"https://api.github.com/repos/Yelp/paasta/collaborators{/collaborator}","teams_url":"https://api.github.com/repos/Yelp/paasta/teams","hooks_url":"https://api.github.com/repos/Yelp/paasta/hooks","issue_events_url":"https://api.github.com/repos/Yelp/paasta/issues/events{/number}","events_url":"https://api.github.com/repos/Yelp/paasta/events","assignees_url":"https://api.github.com/repos/Yelp/paasta/assignees{/user}","branches_url":"https://api.github.com/repos/Yelp/paasta/branches{/branch}","tags_url":"https://api.github.com/repos/Yelp/paasta/tags","blobs_url":"https://api.github.com/repos/Yelp/paasta/git/blobs{/sha}","git_tags_url":"https://api.github.com/repos/Yelp/paasta/git/tags{/sha}","git_refs_url":"https://api.github.com/repos/Yelp/paasta/git/refs{/sha}","trees_url":"https://api.github.com/repos/Yelp/paasta/git/trees{/sha}","statuses_url":"https://api.github.com/repos/Yelp/paasta/statuses/{sha}","languages_url":"https://api.github.com/repos/Yelp/paasta/languages","stargazers_url":"https://api.github.com/repos/Yelp/paasta/stargazers","contributors_url":"https://api.github.com/repos/Yelp/paasta/contributors","subscribers_url":"https://api.github.com/repos/Yelp/paasta/subscribers","subscription_url":"https://api.github.com/repos/Yelp/paasta/subscription","commits_url":"https://api.github.com/repos/Yelp/paasta/commits{/sha}","git_commits_url":"https://api.github.com/repos/Yelp/paasta/git/commits{/sha}","comments_url":"https://api.github.com/repos/Yelp/paasta/comments{/number}","issue_comment_url":"https://api.github.com/repos/Yelp/paasta/issues/comments{/number}","contents_url":"https://api.github.com/repos/Yelp/paasta/contents/{+path}","compare_url":"https://api.github.com/repos/Yelp/paasta/compare/{base}...{head}","merges_url":"https://api.github.com/repos/Yelp/paasta/merges","archive_url":"https://api.github.com/repos/Yelp/paasta/{archive_format}{/ref}","downloads_url":"https://api.github.com/repos/Yelp/paasta/downloads","issues_url":"https://api.github.com/repos/Yelp/paasta/issues{/number}","pulls_url":"https://api.github.com/repos/Yelp/paasta/pulls{/number}","milestones_url":"https://api.github.com/repos/Yelp/paasta/milestones{/number}","notifications_url":"https://api.github.com/repos/Yelp/paasta/notifications{?since,all,participating}","labels_url":"https://api.github.com/repos/Yelp/paasta/labels{/name}","releases_url":"https://api.github.com/repos/Yelp/paasta/releases{/id}","deployments_url":"https://api.github.com/repos/Yelp/paasta/deployments","created_at":"2015-10-26T21:35:53Z","updated_at":"2019-09-04T21:28:48Z","pushed_at":"2019-09-05T14:04:26Z","git_url":"git://github.com/Yelp/paasta.git","ssh_url":"git@github.com:Yelp/paasta.git","clone_url":"https://github.com/Yelp/paasta.git","svn_url":"https://github.com/Yelp/paasta","homepage":"","size":64704,"stargazers_count":1364,"watchers_count":1364,"language":"Python","has_issues":true,"has_projects":true,"has_downloads":true,"has_wiki":false,"has_pages":false,"forks_count":169,"mirror_url":null,"archived":false,"disabled":false,"open_issues_count":96,"license":{"key":"apache-2.0","name":"Apache License 2.0","spdx_id":"Apache-2.0","url":"https://api.github.com/licenses/apache-2.0","node_id":"MDc6TGljZW5zZTI="},"forks":169,"open_issues":96,"watchers":1364,"default_branch":"master"}},"_links":{"self":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/2403"},"html":{"href":"https://github.com/Yelp/paasta/pull/2403"},"issue":{"href":"https://api.github.com/repos/Yelp/paasta/issues/2403"},"comments":{"href":"https://api.github.com/repos/Yelp/paasta/issues/2403/comments"},"review_comments":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/2403/comments"},"review_comment":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/comments{/number}"},"commits":{"href":"https://api.github.com/repos/Yelp/paasta/pulls/2403/commits"},"statuses":{"href":"https://api.github.com/repos/Yelp/paasta/statuses/5931f29c70f8392071513338efe44dc7bc58969d"}},"author_association":"CONTRIBUTOR"}}
other: {"actor":{"display_login":"stug"}}