Skip to content

fix: download timeouts#6

Draft
sveitser wants to merge 5 commits intomainfrom
ma/retry-fixup
Draft

fix: download timeouts#6
sveitser wants to merge 5 commits intomainfrom
ma/retry-fixup

Conversation

@sveitser
Copy link

No description provided.

Extract download parameters into DownloadConfig struct, stream
responses directly to disk instead of buffering in memory, and add
configurable connect/read timeouts with corresponding tests.
@gemini-code-assist
Copy link

Summary of Changes

Hello, 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 potential download timeouts by implementing configurable connection and read timeouts for HTTP requests. It introduces a dedicated configuration struct to manage these parameters, refactors the download utility to stream data more efficiently, and includes new tests to validate the timeout mechanisms. These changes enhance the reliability and performance of file downloads within the system.

Highlights

  • Configurable Download Timeouts: Introduced a new DownloadConfig struct to centralize and manage download parameters, including connect_timeout and read_timeout for HTTP requests. This allows for fine-grained control over how long download operations will wait for connections and data.
  • Improved Download Mechanism: Refactored the download_url_to_file function to accept the new DownloadConfig and to stream HTTP responses directly to the target file. This change optimizes memory usage by avoiding intermediate buffers for large downloads.
  • Enhanced Test Coverage: Added new unit tests specifically for connection and read timeouts, ensuring the robustness of the download functionality under various network conditions.
  • Dependency Update: Updated the ark-srs dependency to version 0.3.4 in Cargo.lock.

🧠 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.

Changelog
  • Cargo.lock
    • Updated ark-srs dependency to version 0.3.4.
  • src/load.rs
    • Corrected a typo in a module-level comment from 'memroy' to 'memory'.
    • Introduced DownloadConfig struct with max_retries, base_backoff, connect_timeout, and read_timeout fields.
    • Added Default implementation for DownloadConfig with sensible timeout values.
    • Provided #[cfg(test)] builder methods for DownloadConfig to facilitate testing.
    • Modified download_url_to_file to accept &DownloadConfig instead of individual parameters.
    • Implemented ureq::AgentBuilder to apply connect_timeout and read_timeout to HTTP requests.
    • Changed download logic to stream response body directly to the file using std::io::copy.
    • Updated download_srs_file to use DownloadConfig::default().
    • Added std::net::TcpListener import for new tests.
    • Updated existing tests to utilize the new DownloadConfig struct.
    • Added test_connect_timeout_returns_error to verify connection timeout functionality.
    • Added test_read_timeout_returns_error to verify read timeout functionality.
Ignored Files
  • Ignored by pattern: .github/workflows/** (1)
    • .github/workflows/ci.yml
Using Gemini Code Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces timeouts for download operations by refactoring the download logic to use a DownloadConfig struct and a ureq::Agent. It also improves memory efficiency by streaming downloads directly to a file instead of buffering in memory. The changes are well-structured and include new tests for the timeout functionality.

I've found one high-severity issue where the new implementation fails to retry on I/O errors during the download stream, which is a regression from the previous behavior. A code suggestion is provided to fix this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant