-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Livecheck: Replace OpenURI#open with Curl #10834
Livecheck: Replace OpenURI#open with Curl #10834
Conversation
Review period will end on 2021-03-12 at 17:04:38 UTC. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to separate the curl
refactoring from the download_strategy
changes from the livecheck
changes so this is 2-3 PRs instead of 1. Otherwise: looking good so far!
I managed to reproduce a hang with this backtrace:
|
That |
Review period ended. |
I have that specific timeout for |
Makes sense to me. It's possible to create a PR for the additional
@reitermarkus Do you have time to separate the related |
I think it would be good to do any refactoring in a dedicated PR that should result in no changes being required to callers of the refactored code. |
@samford Anything blocking here? |
It seems to me that the timeout solution proposed is targeted at We however don't need to call |
I'm working on wrapping up some refactoring of the This PR depends on the aforementioned The I'll rebase this on #10990 and test it as well, to see whether it also encounters this new hang. I'll try to get the |
The only other issue I'm aware of is one where the pipe outlives the process, though it would be odd if that affected This is seen with Homebrew/homebrew-cask#32364. I do know how to fix it: instead of waiting on the pipe to close we would wait on the process to close and stop processing after that. This does require pushing the pipe handling code into its own thread but I do have it working locally and can confirm it fixes the issue in Homebrew/homebrew-cask#32364. |
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
b3a0466
to
4ea850f
Compare
4ea850f
to
7fe8028
Compare
To help this move forward to being merged (with respect to #11252 (comment)), I've replaced usage of the methods from #11252 with the previous code that duplicates the existing logic for separating the heads/body from curl output in The duplicated code works for now and will be replaced in #11252, so I don't think we should fret over the I've continued to test this regularly and I haven't seen an unresolved hang after Markus and Bo's work was merged. Instead of hanging, we receive a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Almost there.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
7fe8028
to
c087d48
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your work on this. Almost there!
max_iterations = 5 | ||
iterations = 0 | ||
output = output.lstrip | ||
while output.match?(%r{\AHTTP/[\d.]+ \d+}) && output.include?("\r\n\r\n") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why "\r\n\r\n"
? Could that get a comment (perhaps with a variable/constant extraction, too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double CRLF
(\r\n\r\n
) is used to separate response head(s) and body, whereas a single CRLF
(\r\n
) is used to separate headers in a given head (i.e., the status line and each header line from one response). For example, if the output involves multiple redirections, you'll have multiple head responses with each separated by double CRLF
. Regardless of the number of heads, the final head is also separated from the body by a double CRLF
.
The body content can also contain \r\n\r\n
, so this is why I've taken the approach of using lstrip
and checking the start of the text to ensure it's an HTTP status line.
That said, it's technically possible for a server to use \n\n
(instead of \r\n\r\n
) as the separator. It's very rare but I've read that some folks have encountered this in practice (if Stack Overflow answers/comments are any indicator).
Properly separating the head(s) and body under a variety of conditions is a challenge and we only have to do this because we're working with the output from the curl
application. Is there any particular reason why we do this instead of using a gem that wraps libcurl
(e.g., Typhoeus/Ethon or Curb)?
libcurl
can separate the head(s) from the body for you, so we wouldn't have to worry about parsing the CLI output and hoping a weird server doesn't break it someday. This would simplify usage and allow us to avoid the additional methods in #11252.
If that's something we're interested in, I would be more than happy to work on it (in a follow-up PR, of course).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Properly separating the head(s) and body under a variety of conditions is a challenge and we only have to do this because we're working with the output from the
curl
application. Is there any particular reason why we do this instead of using a gem that wrapslibcurl
(e.g., Typhoeus/Ethon or Curb)?
- consistency with formulae downloads (which have always shelled out)
- avoiding the need to have gems that require compilation for end users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm content to shelve the libcurl
idea for now (so many other things to work on) but just for the sake of clarification: by "gems that require compilation for end users" do you mean building native extensions or something else? Typhoeus/Ethon don't have to build native extensions but Curb does, if that matters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, that's interesting. Yes, it might be interesting to explore those gems if they provided benefits to us, then.
c087d48
to
b907039
Compare
b907039
to
a6769ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work here, super readable now. Thanks @samford!
@MikeMcQuaid Thanks for all your feedback on this (and patience as this progressed over time). Also big thanks to Markus and Bo for resolving the persistent hang that was holding this up for some time. |
@samford you're very welcome. team work, dream work, etc. 😁 |
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?This is a recreation of #9535, as that PR somehow lost its association with the related branch in my fork and the PR stopped reflecting new pushes.
This migrates
brew livecheck
fromopen-uri
toUtils::Curl
, asCurl
is used throughout brew for network requests and livecheck is the only place whereopen-uri
is used (outside of a vendored gem).Notable Changes
URI.parse(url).open
inLivecheck::Strategy#page_content
with#curl_with_workarounds
. Strategies use#page_content
to fetch a page, so this effectively replacesopen-uri
in livecheck withUtils::Curl
.