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.

Sign up for GitHub

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

Jump to bottom

url_downloader: Directly use network cache when downloading data #834

Merged
merged 2 commits into from Jun 14, 2019
Merged

url_downloader: Directly use network cache when downloading data #834

merged 2 commits into from Jun 14, 2019

Conversation

townsend2010
Copy link
Collaborator

Due to how QNetwork handles caching via QNetworkAccessManager and issues it
causes, instead directly access the network cache. This includes comparing the
Last-Modified header of the URL to what is cached and also use cached data (if
availble) when there is an issue downloading the data from the URL.

Fixes #825, fixes #829

@townsend2010
url_downloader: Directly use network cache when downloading data
37d5434 
Due to how QNetwork handles caching via QNetworkAccessManager and issues it
causes, instead directly access the network cache. This includes comparing the
Last-Modified header of the URL to what is cached and also use cached data (if
availble) when there is an issue downloading the data from the URL.

Fixes #825, fixes #829
@codecov
Copy link

codecov bot commented Jun 11, 2019 •
edited

Codecov Report

Merging #834 into master will decrease coverage by 0.06%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #834      +/-   ##
==========================================
- Coverage   68.69%   68.63%   -0.07%     
==========================================
  Files         191      191              
  Lines        6526     6551      +25     
==========================================
+ Hits         4483     4496      +13     
- Misses       2043     2055      +12
Impacted Files Coverage Δ
src/network/url_downloader.cpp 56.38% <36.36%> (-6.64%) ⬇️
src/daemon/daemon.cpp 29.24% <0%> (-0.06%) ⬇️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 62.6% <0%> (+2.08%) ⬆️

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 41e1e54...857c56d. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 11, 2019

Codecov Report

Merging #834 into master will decrease coverage by 0.13%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #834      +/-   ##
==========================================
- Coverage   68.69%   68.56%   -0.14%     
==========================================
  Files         191      191              
  Lines        6526     6549      +23     
==========================================
+ Hits         4483     4490       +7     
- Misses       2043     2059      +16
Impacted Files Coverage Δ
src/network/url_downloader.cpp 55.2% <33.33%> (-7.81%) ⬇️

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 41e1e54...37d5434. Read the comment docs.

@Saviq
Copy link
Collaborator

Saviq commented Jun 11, 2019

Hey, could we have tests for this?

@townsend2010
Copy link
Collaborator Author

@Saviq,

I thought about it and we would need to mock things like QNetworkDiskManager and be able to populate it with cache data in a way that works for QAbstractNetworkCache. I didn't see any straightforward way to do that, although thinking about it as I write this, I wonder if it's possible to do something with our premock stuff we use for testing. I'll look into that before declaring that this would take too much time for a small gain.

@townsend2010
Copy link
Collaborator Author

Ok doesn't look like premock will help us on this. So basically, what would need to be done is pre-populate a cache that is understood by QNetworkDiskCache. This would be fragile since in the Qt code, they use internal versioning of the cache and use obscure file names along with internal metadata in the cache files we'd have to get correct.

I can add this basing this on the real cache on my system, but it could be prone to breakage down the line if Qt decides to change how it handles the cache internally, including path versioning that it uses and special metadata in the cache file. I'm just not sure it's worth it.

@townsend2010
Copy link
Collaborator Author

After some more looking at this, I really don't think it's worth the effort to put in the static caching infrastructure (which would potentially be fragile to Qt changes) in place for testing this small chunk of code.

gerboland
gerboland reviewed Jun 13, 2019
View reviewed changes
Copy link
Contributor

@gerboland gerboland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small comment. Code makes sense, but I'm not sure how to test it. It does work while I'm offline at least

src/network/url_downloader.cpp Outdated Show resolved Hide resolved
gerboland
gerboland approved these changes Jun 13, 2019
View reviewed changes
Copy link
Contributor

@gerboland gerboland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it's so hard to test, but code looks reasonable, I'm ok with landing this and then watching how it behaves in use for a while.

@townsend2010
Copy link
Collaborator Author

Shall we merge?

@gerboland
Copy link
Contributor

Nobody else has opinion then? Ok, then
bors r+

bors bot added a commit that referenced this pull request Jun 14, 2019
@bors @townsend2010
Merge #834
af773b8 
834: url_downloader: Directly use network cache when downloading data r=gerboland a=townsend2010

Due to how QNetwork handles caching via QNetworkAccessManager and issues it
causes, instead directly access the network cache. This includes comparing the
Last-Modified header of the URL to what is cached and also use cached data (if
availble) when there is an issue downloading the data from the URL.

Fixes #825, fixes #829

Co-authored-by: Chris Townsend <christopher.townsend@canonical.com>
@bors
Copy link
Contributor

bors bot commented Jun 14, 2019

Build succeeded

@bors bors bot merged commit 857c56d into master Jun 14, 2019
@bors bors bot deleted the handle-network-cache-better branch June 14, 2019 13:10
@Saviq
Copy link
Collaborator

Saviq commented Jun 14, 2019 •
edited

Well, I do have an opinion.

🎵 EverythingThis is awesome! 🎵

Saviq pushed a commit that referenced this pull request Jun 17, 2019
@bors @townsend2010 @Saviq
Merge #834
a5a157b 
834: url_downloader: Directly use network cache when downloading data r=gerboland a=townsend2010

Due to how QNetwork handles caching via QNetworkAccessManager and issues it
causes, instead directly access the network cache. This includes comparing the
Last-Modified header of the URL to what is cached and also use cached data (if
availble) when there is an issue downloading the data from the URL.

Fixes #825, fixes #829

Co-authored-by: Chris Townsend <christopher.townsend@canonical.com>
@townsend2010 townsend2010 mentioned this pull request Jul 24, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gerboland gerboland gerboland approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@townsend2010 @Saviq @gerboland