fix(python): Add timeout to ErrorDocHelper requests.get() calls#1661
fix(python): Add timeout to ErrorDocHelper requests.get() calls#1661maxlillo wants to merge 1 commit intolooker-open-source:mainfrom
Conversation
The requests.get() calls in ErrorDocHelper had no timeout parameter, which could cause Python scripts using the SDK to hang indefinitely when an API error occurs and the error documentation CDN is unresponsive. This adds a configurable timeout parameter (default 10 seconds) to: - get_index() - lookup() - parse_and_lookup() 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the robustness of the SDK's error handling by addressing a critical vulnerability where the application could become unresponsive if the error documentation CDN failed to respond. By introducing a default 10-second timeout to all network requests made for error documentation, the system is now designed to fail gracefully, preventing indefinite blocking and improving overall application stability. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request correctly adds a timeout parameter to requests.get() calls in ErrorDocHelper, which is an important step to prevent the application from hanging when the error documentation CDN is unresponsive. My review includes a couple of suggestions to further improve the robustness of these network requests by adding checks for unsuccessful HTTP responses. This will ensure that errors like 404s or 500s are also handled gracefully.
| r = requests.get(f"{url}index.json", timeout=timeout) | ||
| self.lookup_dict = json.loads(r.text) |
There was a problem hiding this comment.
The requests.get() call doesn't check for unsuccessful HTTP status codes. If the server returns an error (e.g., 404, 500), r.text might not be valid JSON, which could lead to a json.JSONDecodeError. It's a good practice to handle such responses explicitly.
I suggest using r.raise_for_status() to raise an HTTPError for bad responses. This exception is a subclass of requests.exceptions.RequestException and will be caught by the handler in parse_and_lookup, ensuring graceful failure. Using r.json() is also more idiomatic for parsing JSON responses.
| r = requests.get(f"{url}index.json", timeout=timeout) | |
| self.lookup_dict = json.loads(r.text) | |
| r = requests.get(f"{url}index.json", timeout=timeout) | |
| r.raise_for_status() | |
| self.lookup_dict = r.json() |
| r = requests.get(f"{self.ERROR_CODES_URL}{error_doc_url}", timeout=timeout) | ||
| error_doc = r.text |
There was a problem hiding this comment.
Similar to the other requests.get() call, this one doesn't handle non-200 HTTP status codes. If the request fails, r.text could contain an error message (e.g., an HTML page) instead of the expected markdown documentation. To make this more robust, it's best to check the response status.
Adding r.raise_for_status() will ensure the request was successful before its content is used. The resulting exception will be handled gracefully by the try...except block in parse_and_lookup.
| r = requests.get(f"{self.ERROR_CODES_URL}{error_doc_url}", timeout=timeout) | |
| error_doc = r.text | |
| r = requests.get(f"{self.ERROR_CODES_URL}{error_doc_url}", timeout=timeout) | |
| r.raise_for_status() | |
| error_doc = r.text |
1 participant
Summary
timeoutparameter (default 10 seconds) torequests.get()calls inErrorDocHelperProblem
The
requests.get()calls inpython/looker_sdk/error.pyhad no timeout parameter. When an API error occurs and the SDK attempts to fetch supplemental error documentation fromhttps://static-a.cdn.looker.app/errorcodes/, if the CDN is reachable but unresponsive, the request blocks indefinitely.This causes Python scripts using the SDK to hang when they encounter an API error and the CDN doesn't respond.
Changes
Added
timeoutparameter to:get_index()lookup()parse_and_lookup()All default to 10 seconds, which is consistent with common timeout practices for HTTP requests.
Test plan
🤖 Generated with Claude Code