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

Make image downloads and prepare operations asynchronous #724

Merged
merged 5 commits into from
Apr 19, 2019

Conversation

townsend2010
Copy link
Contributor

No description provided.

Chris Townsend added 4 commits April 15, 2019 10:01
QNetworkAccessManager can only run in the thread that created it, so in order to run network
operations in a different thread, URLDownloader will now allocate a new QNetworkAccessManager
object when needed.
Also update copyrights where appropriate.
@townsend2010 townsend2010 force-pushed the async-image-download-prepare-operation branch from df17d6b to 30475a3 Compare April 16, 2019 16:36
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.

I can confirm this unblocks the daemon while download is happening. But when testing it still looks like "Verifying" and "Configuring" stages are blocking somewhere?

I'm doing a naive test: I set launch going in one shell which starts a download, and repeat "multipass list" in another shell to see if it returns quickly. List appears to block until the Verifying and Configuring steps are complete.

@@ -25,6 +25,7 @@
#include <fmt/format.h>

#include <QMap>
#include <QUrl>
Copy link
Contributor

Choose a reason for hiding this comment

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

Needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, because some other includes that pulled in QUrl in url_downloader.h were removed and now an explicit QUrl include is needed here.

@@ -550,6 +550,7 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> the_config)
get_unique_id(config->data_directory), config->data_directory},
metrics_opt_in{get_metrics_opt_in(config->data_directory)}
{
qRegisterMetaType<VirtualMachineDescription>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think VS will choke on this unless you prefix the Type with its namespace.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ugh, silly namespace rules for different compilers....fixed it though:)

@townsend2010
Copy link
Contributor Author

@gerboland,

I'm puzzled by what you're seeing. I too test the "blocking" behavior as you describe, and I just tried it again and the "Verify" and "Configure" steps are unblocked for me with this branch.

I'm sure you've already done this, but could you please double confirm that you were running the right version of the daemon? Otherwise, I'm not really sure what is going on since the "Download", "Verify", and "Configure" steps are all running in a new (and the same) thread now.

@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #724 into master will increase coverage by 0.06%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #724      +/-   ##
=========================================
+ Coverage   66.83%   66.9%   +0.06%     
=========================================
  Files         174     174              
  Lines        6100    6118      +18     
=========================================
+ Hits         4077    4093      +16     
- Misses       2023    2025       +2
Impacted Files Coverage Δ
include/multipass/url_downloader.h 0% <ø> (ø) ⬆️
src/daemon/custom_image_host.cpp 94.93% <ø> (ø) ⬆️
src/daemon/daemon.h 50% <ø> (ø) ⬆️
...tform/backends/libvirt/libvirt_virtual_machine.cpp 42.38% <0%> (-0.29%) ⬇️
include/multipass/virtual_machine.h 0% <0%> (ø) ⬆️
include/multipass/virtual_machine_description.h 100% <100%> (ø) ⬆️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 60.98% <100%> (ø) ⬆️
src/daemon/daemon.cpp 25.82% <88.33%> (+0.46%) ⬆️
src/network/url_downloader.cpp 62.16% <91.3%> (+3.7%) ⬆️

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 1ae0be7...bae45c8. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Apr 19, 2019

Codecov Report

Merging #724 into master will increase coverage by 0.06%.
The diff coverage is 86.36%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #724      +/-   ##
=========================================
+ Coverage   66.83%   66.9%   +0.06%     
=========================================
  Files         174     174              
  Lines        6100    6118      +18     
=========================================
+ Hits         4077    4093      +16     
- Misses       2023    2025       +2
Impacted Files Coverage Δ
include/multipass/url_downloader.h 0% <ø> (ø) ⬆️
src/daemon/custom_image_host.cpp 94.93% <ø> (ø) ⬆️
src/daemon/daemon.h 50% <ø> (ø) ⬆️
...tform/backends/libvirt/libvirt_virtual_machine.cpp 42.38% <0%> (-0.29%) ⬇️
include/multipass/virtual_machine.h 0% <0%> (ø) ⬆️
include/multipass/virtual_machine_description.h 100% <100%> (ø) ⬆️
...rc/platform/backends/qemu/qemu_virtual_machine.cpp 60.98% <100%> (ø) ⬆️
src/daemon/daemon.cpp 25.82% <88.33%> (+0.46%) ⬆️
src/network/url_downloader.cpp 62.16% <91.3%> (+3.7%) ⬆️

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 1ae0be7...bae45c8. Read the comment docs.

@gerboland
Copy link
Contributor

@townsend2010 totally my bad, I was flicking between master and this to see if another bug was introduced by this, and forgot to call make.

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.

lgtm
bors r+

bors bot added a commit that referenced this pull request Apr 19, 2019
724: Make image downloads and prepare operations asynchronous r=gerboland a=townsend2010



Co-authored-by: Chris Townsend <[email protected]>
@townsend2010
Copy link
Contributor Author

@gerboland,

Sweet! Glad that's all it was:) Thanks!

@bors
Copy link
Contributor

bors bot commented Apr 19, 2019

Build failed

@townsend2010
Copy link
Contributor Author

The bors build failed because the Mac build fails and needs fixing there. I will merge this manually and fix that build elsewhere.

@townsend2010 townsend2010 merged commit bae45c8 into master Apr 19, 2019
@bors bors bot deleted the async-image-download-prepare-operation branch April 19, 2019 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants