Fix CSPICE download timeouts using read_timeout instead of overall timeout#11
Fix CSPICE download timeouts using read_timeout instead of overall timeout#11shunsuke-shimomura wants to merge 5 commits intojacob-pro:masterfrom
Conversation
|
Hi @jacob-pro, I've created this PR to address timeout issues that users have been experiencing when downloading CSPICE archives during the build process. The main improvement is switching from a blocking download to an async streaming implementation with proper timeout configuration. Most importantly, I've used The key benefit is that downloads will only timeout if the connection is genuinely stalled (no data received for 60 seconds), rather than failing active downloads on slower networks. This should significantly improve the build experience for users with limited bandwidth. I'd appreciate your review and feedback on this approach. Please let me know if you'd like any changes or have concerns about the implementation. Thank you for maintaining this project! |
|
Thanks for identifying this @shunsuke-shimomura Yes you are completely correct that read timeouts are much more appropriate for variably sized files, and also that it is better to stream the file to disk rather than buffer the whole response. Presumably you had to switch to the async client because the blocking one does not have a read timeout option? Maybe using ureq (https://github.com/algesten/ureq) instead might be a better alternative? |
|
Hi @jacob-pro, thanks for the response. As you pointed out, I switched to the async client because the blocking one lacks a read_timeout option. It does seem better to use ureq and implement this synchronously, which would reduce its dependency on an async runtime and keep the build lighter, so I’ll give that a try! |
|
@jacob-pro |
| .expect("Failed to start CSPICE download"); | ||
|
|
||
| // Check status code | ||
| if response.status() < 200 || response.status() >= 300 { |
There was a problem hiding this comment.
should be if response.status() != 200 ?
I can't think of any other valid codes in this scenario
| downloaded += n as u64; | ||
|
|
||
| // Progress display (every 1MB) | ||
| if downloaded % (1024 * 1024) < n as u64 { |
There was a problem hiding this comment.
please can you use https://github.com/console-rs/indicatif instead of implementing this yourself?
| } | ||
|
|
||
| // Get content size | ||
| let total_size = response |
There was a problem hiding this comment.
|
|
||
| file.flush().expect("Failed to flush download file"); | ||
|
|
||
| // Verify download completion |
There was a problem hiding this comment.
is there any need to check this? If any problems happen during the download it should panic above.
ureq will also raise an error if the stream terminates earlier than the declared content length
https://docs.rs/ureq/latest/ureq/struct.Body.html#body-lengths
Description
This PR fixes a issue where CSPICE downloads were failing due to inappropriate timeout configurations. The previous implementation could timeout on large files even when the download was progressing normally.
Problem
The blocking download implementation had no proper timeout control, which could cause:
Solution
Implemented async streaming download with read_timeout instead of an overall timeout:
read_timeout(60s): Maximum time to wait for the next chunk of data
connect_timeout(30s): Maximum time to establish initial connection
No overall timeout: Allows large downloads to complete on slow connections
Why read_timeout is crucial:
This ensures the download only fails when the connection is genuinely stalled (no data for 60s), not when it's simply slow but making progress.
Additional improvements: