Curl: Remove guard from certain parsing logic #13195
Merged
Conversation
This file contains 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
The `#curl_http_content_headers_and_checksum` method previously parsed responses from `curl` output even if `status.success?` wasn't `true`. A recent commit of mine moved the parsing logic behind this guard but it's now leading to a "...is not reachable" error when a URL involves a large download that takes longer than 25 seconds to finish and hits the timeout. This commit resolves the issue for the time being by moving related logic back to its previous location, where it isn't guarded by `status.success?`.
samford
added
the
critical
|
Review period skipped due to |
BrewTestBot
approved these changes
Apr 26, 2022
This was referenced Apr 26, 2022
|
Thanks @samford. I appreciate you getting to this as quickly as you have! |
MikeMcQuaid
approved these changes
Apr 26, 2022
samford
deleted the
curl_http_content_headers_and_checksum-reinstate-old-return-behavior
branch
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?@bevanjkay brought it to my attention that some cask PRs started failing on CI with a
The binary url ... is not reachableerror after #11252 was merged. This error message comes from theUtils::Curl#curl_check_http_contentmethod, which calls#curl_http_content_headers_and_checksum. Both of these methods were modified in that PR but the issue arises from the latter.Long story short, I naively moved the response parsing logic in
#curl_http_content_headers_and_checksuminto theif status.success?block and this change causes this error to arise when the download for a URL doesn't finish before the 25 second timeout. Previously, this method would parse the output regardless of whether a final response arrived and this would technically work if there were other responses in the output (e.g., redirections).This PR reinstates the previous behavior by moving the parsing logic back out of the
if status.success?block, which resolves this issue. We may want to look into this further after this is merged but I wanted to get this fix out relatively quickly, to unblock related cask PRs.I was able to replicate this issue locally (and confirm the fix) using the
visual-paradigmcask (related open PR: Homebrew/homebrew-cask#122490) because the dmg file is ~750 MB and this will trigger thecurltimeout. You can test this locally usingbrew audit --cask --appcast --online visual-paradigm.I've labelled this as "critical", since it's a bug fix. I did a limited amount of testing and didn't see any new errors but if anyone wants to test this more before merging, please feel free. It's very late over here and I'm crawling into bed but I can check on this again in the morning (EDT).