Improve dotnetup downloads timeout#54754
Draft
nagilson wants to merge 3 commits into
Draft
Conversation
In dotnet#54752 (review), @MichaelSimons observed a ci run where dotnetup got stuck for 2 hours and the 10 minute download timeout did not seem to be respected. What we should add is an explicit download-body timeout, preferably one of these: Idle/no-progress timeout: cancel if no bytes are read for, say, 2-5 minutes. Overall archive download timeout: cancel the whole archive transfer after some max duration. Both: overall cap plus idle timeout, with the idle timeout being the important fix for this CI case. Changes: Added configurable timeout settings in DownloadTimeoutSettings.cs DOTNETUP_DOWNLOAD_IDLE_TIMEOUT_SECONDS, default 120 DOTNETUP_DOWNLOAD_TIMEOUT_SECONDS, default 900 Invalid, zero, or negative values fall back to defaults. Updated DotnetArchiveDownloader.cs Total timeout now covers the full archive download across retries. Idle timeout now wraps each body stream read, so a stuck ReadAsync fails instead of hanging forever. Blob-feed .sha512 checksum fetches are also bounded by the total timeout. Updated DefaultHttpClient.cs Removed the hidden 10-minute HttpClient.Timeout cap so the new configurable total timeout is the controlling budget. Kept timeout ownership at callers via cancellation tokens. Added focused tests in DotnetArchiveDownloaderTests.cs and DownloadTimeoutSettingsTests.cs, plus small test helpers under Utilities. # Not implemented [Dotnetup: Use DefaultHttpClient in DailyChannelResolver · Issue dotnet#54477 · dotnet/sdk](dotnet#54477) to reduce http client code duplication in this change as well as a separate commit - **Daily channel aka.ms resolver** does not use the standard http client and so it would not have this instrumentation.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #54752 (review), @MichaelSimons observed a ci run where dotnetup got stuck for 2 hours and the 10 minute download timeout did not seem to be respected.
What we should add is an explicit download-body timeout:
Idle/no-progress timeout: cancel if no bytes are read for, say, 2-5 minutes.
Overall archive download timeout: cancel the whole archive transfer after some max duration.
Changes:
Added configurable timeout settings in DownloadTimeoutSettings.cs DOTNETUP_DOWNLOAD_IDLE_TIMEOUT_SECONDS, default 120 DOTNETUP_DOWNLOAD_TIMEOUT_SECONDS, default 900
Invalid, zero, or negative values fall back to defaults. Updated DotnetArchiveDownloader.cs
Total timeout now covers the full archive download across retries. Idle timeout now wraps each body stream read, so a stuck ReadAsync fails instead of hanging forever. Blob-feed .sha512 checksum fetches are also bounded by the total timeout. Updated DefaultHttpClient.cs
Removed the hidden 10-minute HttpClient.Timeout cap so the new configurable total timeout is the controlling budget. Kept timeout ownership at callers via cancellation tokens. Added focused tests in DotnetArchiveDownloaderTests.cs and DownloadTimeoutSettingsTests.cs, plus small test helpers under Utilities.
Not implemented
Dotnetup: Use DefaultHttpClient in DailyChannelResolver · Issue #54477 · dotnet/sdk to reduce http client code duplication in this change as well as a separate commit - Daily channel aka.ms resolver does not use the standard http client and so it would not have this instrumentation.