Skip to content
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

Avoid downloading data when rehashing URL #982

Merged
merged 6 commits into from
Aug 9, 2019

Conversation

noamross
Copy link
Contributor

@noamross noamross commented Aug 8, 2019

Summary

It seems that current behavior for rehashing URL still downloads the whole contents of that URL to memory. This change avoids doing so at all by telling curl to only get response headers, no body.

I'm not sure where or how in the test framework to set up a test for "Does not download the whole URL content body when testing for changes at the URL."

Related GitHub issues and pull requests

  • Ref: #

Checklist

It seems that current behavior for rehashing  URL still downloads the whole contents of that URL to memory.  This change avoids doing so at all by telling `curl` to only get response headers, no body.
@noamross noamross mentioned this pull request Aug 8, 2019
2 tasks
@wlandau
Copy link
Member

wlandau commented Aug 8, 2019

Thanks for catching this, @noamross. Would you mention it in NEWS.md?

For testing, I think we could mock assert_useful_headers() in rehash_url() or assert_useful_headers() to make sure we download just the headers.

@wlandau
Copy link
Member

wlandau commented Aug 8, 2019

Hmm... instead of mockr, what about stopifnot(length(req$content) < 1L) just after req <- curl::curl_fetch_memory(url, handle = curl::new_handle(nobody = TRUE)) in rehash_url()?

@wlandau
Copy link
Member

wlandau commented Aug 8, 2019

A couple more comments:

  1. Don't worry about the Travis failure. I am almost positive it is caused by 'UseMethod' used in an inappropriate fashion r-lib/covr#392. Feel free to switch back to CRAN covr in .travis.yml.
  2. If you would like, please feel free to add yourself as a contributor in the DESCRIPTION. It's a small PR, but it uncovered an important performance issue.

@wlandau
Copy link
Member

wlandau commented Aug 9, 2019

In fact, for (1), I will rerun Travis since the issue with covr should now be fixed.

@codecov-io
Copy link

codecov-io commented Aug 9, 2019

Codecov Report

Merging #982 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #982   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          46     46           
  Lines        6336   6337    +1     
=====================================
+ Hits         6336   6337    +1
Impacted Files Coverage Δ
R/drake_meta_.R 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ad122a7...5bd3d69. Read the comment docs.

@wlandau
Copy link
Member

wlandau commented Aug 9, 2019

Looks great, Noam. Thanks again!

@wlandau wlandau merged commit 111cf65 into ropensci:master Aug 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants