René's URL Explorer Experiment

{"@context":"https://schema.org","@type":"DiscussionForumPosting","headline":"async retries timeout race condition","articleBody":"### Problem\r\n\r\nAsync retries use an [`asyncio.wait_for` line](https://github.com/googleapis/python-api-core/blob/bdebd6331f9c0d3d1a8ceaf274f07d2ed75bfe92/google/api_core/retry_async.py#L121) to break out early when the deadline is hit. The problem is, in practice, retries typically wrap gapic grpc calls, which implement their own timeout logic. This means there is a natural race condition when the timeout is reached, where one of two things can happen:\r\n- the `wait_for` line in the wrapper notices the timeout first:\r\n    - the wrapper emitts a `RetryError`\r\n    -  a CancelledError is sent down to the wrapped function, which the wrapped function may have to account for properly\r\n- The wrapped grpc call times out on its own:\r\n    - a `DeadlineExceeded` error is passed to the wrapper\r\n        - This may result in the wrapper raising a `RetryError`, if DeadlineExceeded is in the list of retryable errors, otherwise it will be raised directly through the wrapper. \r\n\r\nOn top of the race condition, there are a couple other issues with wait_for:\r\n- `wait_for` creates a new task, which can add significant performance overhead\r\n- This behaviour differs from the sync retry function, which only checks for timeouts after the wrapped function attempt is complete\r\n\r\n---\r\n### Solutions:\r\n\r\nWe can add a new `interrupt_on_timeout` argument to the async retry function, and only enable wait_for when the flag is True. To avoid breaking changes, we can have the flag default to True\r\n\r\nBut personally, I think we should consider just removing the wait_for entirely\r\n\r\n---\r\n\r\n### Streaming Retries:\r\n\r\n[Streaming retries](https://github.com/googleapis/python-api-core/pull/495) add an extra complication, since there's the added possibility to check for expired timeouts after each yield in the stream, on top of the option to break the stream mid-yield. We we could add another flag to control for to control these timeout options. \r\n\r\nBut adding any extra checks in between each yield can significantly harm performance, especially if we add a wait_for to interrupt mid-yield, like async_retry currently does. For this reason. I suggest that we keep these timeout options out of streaming retries for now, and only keep the async_retries wait_for for compatibility reasons.\r\n\r\nIn general, I think the retry wrappers should expect the wrapped function to handle timeout intrruptions on their own, and the wrappers should only handle timeouts between attempts\r\n","author":{"url":"https://github.com/daniel-sanche","@type":"Person","name":"daniel-sanche"},"datePublished":"2023-09-07T20:56:35.000Z","interactionStatistic":{"@type":"InteractionCounter","interactionType":"https://schema.org/CommentAction","userInteractionCount":1},"url":"https://github.com/528/python-api-core/issues/528"}