fix(client): add connect and request timeouts#537
fix(client): add connect and request timeouts#537dumko2001 wants to merge 7 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: ac994ca The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 addresses a critical reliability issue where the CLI could become unresponsive due to stalled Google API streams or dropped connections. By introducing explicit connection and request timeouts to the HTTP client, the changes ensure that network operations will eventually fail gracefully instead of hanging indefinitely, significantly improving the robustness of the application. Highlights
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
|
![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 introduces connection and request timeouts for the HTTP client, a valuable change to prevent the application from hanging. My review includes one suggestion to improve code maintainability by replacing the hardcoded timeout values with named constants. This aligns with existing patterns in the file and makes the configuration clearer and easier to manage.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds connection and request timeouts to the HTTP client to prevent the CLI from hanging, which is a valuable improvement. The implementation is straightforward. However, the hardcoded 120-second request timeout may be too aggressive for large file transfers, potentially causing legitimate operations to fail. I've suggested increasing this value to better accommodate such use cases while still mitigating the risk of indefinite hangs.
|
Updated to use named constants ( |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces connection and request timeouts to the HTTP client to prevent hangs. While adding a connection timeout is a a solid improvement, the global request timeout could unintentionally break long-running operations like large file downloads, as it applies to the entire request/response lifecycle. I've kept the critical comment with a suggestion to remove the global request timeout and instead encourage applying timeouts on a per-request basis where needed. If this change is made, the changeset file should also be updated to reflect that only a connect timeout was added.
| reqwest::Client::builder() | ||
| .default_headers(headers) | ||
| .connect_timeout(std::time::Duration::from_secs(CONNECT_TIMEOUT_SECS)) | ||
| .timeout(std::time::Duration::from_secs(REQUEST_TIMEOUT_SECS)) |
There was a problem hiding this comment.
Setting a global request timeout on the reqwest::Client can cause issues with long-running operations, such as downloading large files or handling long-lived API streams. The timeout applies to the entire duration of the request, including reading the response body. This means any operation that legitimately takes longer than 120 seconds will fail, which could be a regression for some users.
To avoid breaking this functionality while still preventing indefinite hangs on most API calls, I recommend removing this global timeout from the client (and the corresponding REQUEST_TIMEOUT_SECS constant). Timeouts can then be applied on a per-request basis where appropriate (i.e., for API calls not expected to be long-running).
The connect_timeout is a great addition and doesn't have this side effect, so it should be kept.
|
Removed the global request timeout as suggested. This avoids breaking long-running operations like large file downloads while still providing protection against connection hangs via the 10s |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request aims to prevent the CLI from hanging by adding connection and request timeouts to the HTTP client. The implementation successfully adds a 10-second connection timeout. However, the 120-second request timeout mentioned in the description is missing. I've provided a review comment to add the request timeout to fully address the issue of hanging on stalled streams. This is a critical part of the fix and should be included.
| .connect_timeout(std::time::Duration::from_secs(CONNECT_TIMEOUT_SECS)) | ||
| .build() |
There was a problem hiding this comment.
The pull request description mentions adding a 120-second request timeout, but this change is missing from the implementation. Adding a request timeout is crucial to prevent the client from hanging indefinitely when a stream stalls, which is one of the goals of this PR.
I've added the request timeout to the client builder. Please also define a REQUEST_TIMEOUT_SECS constant for the value 120 at the top of the file, similar to how CONNECT_TIMEOUT_SECS is defined.
Once this is done, please also update the changeset message in .changeset/http-timeouts.md to reflect the full scope of the changes.
.connect_timeout(std::time::Duration::from_secs(CONNECT_TIMEOUT_SECS))
.timeout(std::time::Duration::from_secs(120))
.build()
|
Re-added a 10-minute request timeout to prevent infinite hangs while still accommodating large file transfers, as suggested. |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds connection and request timeouts to the HTTP client to prevent indefinite hangs. While the connection timeout is a beneficial addition, the global request timeout of 10 minutes could unintentionally break functionality for large file downloads or long-running API streams, as it applies to the entire request/response lifecycle. I have left a critical comment with a more detailed explanation and a suggestion to address this potential issue.
| reqwest::Client::builder() | ||
| .default_headers(headers) | ||
| .connect_timeout(std::time::Duration::from_secs(CONNECT_TIMEOUT_SECS)) | ||
| .timeout(std::time::Duration::from_secs(REQUEST_TIMEOUT_SECS)) |
There was a problem hiding this comment.
The global request timeout set with .timeout() might unintentionally break support for large file operations and long-running API streams.
According to the reqwest documentation, this timeout applies to the entire request-response lifecycle. For file downloads, this includes the time to download the complete file, and for streaming APIs, it applies to the entire duration of the stream.
A large file download (e.g., 1GB over a 10 Mbps connection) could take over 10 minutes, causing it to fail. Similarly, any API stream that needs to stay open for more than 10 minutes would be terminated. This seems to contradict the PR's goal of allowing these operations to complete successfully.
Consider removing the global .timeout() from the client builder for now. The connect_timeout is sufficient to prevent hangs on initial connection. A more robust solution for request timeouts would be to apply them on a per-request basis where appropriate, outside of the shared client configuration.
2 participants
Description
Adds a 10-second connection timeout and a generous 10-minute (600s) request timeout to the HTTP client. This prevents the CLI and agents from hanging indefinitely while still allowing large file operations and API streams to complete successfully.
Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.