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

Split COM install command into download and install stages #1528

Merged
merged 14 commits into from
Oct 13, 2021

Conversation

florelis
Copy link
Member

@florelis florelis commented Sep 30, 2021

Based on @sreadingMSFT 's #1431.

This splits the COM install command into download and install stages as a first step for implementing concurrent downloads.

  • Added check for already existing files when downloading.
  • Items in the context orchestrator queue now can contain multiple commands. After executing a queue item, it is immediately requeued if it contains more commands.
  • Queue items for install contain a download command and an install command. The download command executes up to downloading the file. The install command executes starting from downloading the file, which should simply see that it already exists unless it was modified or deleted.
  • Changed removal of motw to prevent it from failing if motw is already not present. This can now happen because a file may go through the removal twice if it is reused.

Still pending:

  • Update thread management logic to do downloads in parallel
  • Improve the hash re-check to consider the installer file renaming to be able to reuse after failures
Microsoft Reviewers: Open in CodeFlow

@florelis florelis requested a review from a team as a code owner September 30, 2021 00:22
{
std::lock_guard<std::mutex> lockQueue{ m_queueLock };

item.SetState(OrchestratorQueueItemState::Queued);
Copy link
Contributor

Choose a reason for hiding this comment

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

item.SetState(OrchestratorQueueItemState::Queued);

Just a note here for when you do try to enable multiple downloads - one case my code didn't attempt to handle is that it doesn't create a new thread when doing a requeue when it maybe should.
Example scenario:
Simultaneous download limit is 4 and 4 apps are downloading while 2 other apps are waiting.
One of the apps finishes downloading and is requeued. Now that one of the apps is done downloading it probably should create a new thread to do another download to keep it at the 4 download limit while the current thread is installing. If the code doesn't create a new thread here then once the simultaneous download stuff comes in you can end up in a case where the queue looks like:

  1. Installing, 2. Downloaded but waiting for Install spot, 3. Downloaded but waiting for Install spot, 4. Downloaded but waiting for install spot, 5. Not started, 6. Not Started.
    which seems wrong.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I hadn't thought of that case. I'll keep it in mind.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah actually I got it wrong. You can't end up with that case above. The worst you can get is three apps downloading while one installs instead of four downloading while one installs, since the threads that see that the install is blocked don't just sit there waiting they'd move on to trying to do a download for the other item. Depending on the download limit that may be ok to just sometimes be doing one less than the max number of downloads since the system will still be busy doing an install (which i think for some installers also involves some downloading). But yeah, something to add a comment about probably on which behavior gets chosen.

src/AppInstallerCommonCore/Downloader.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Commands/COMInstallCommand.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/ContextOrchestrator.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Workflows/InstallFlow.cpp Outdated Show resolved Hide resolved
@ghost ghost added the Needs-Author-Feedback Issue needs attention from issue or PR author label Oct 1, 2021
@ghost ghost removed the Needs-Author-Feedback Issue needs attention from issue or PR author label Oct 1, 2021
sreadingMSFT
sreadingMSFT previously approved these changes Oct 12, 2021
Copy link
Contributor

@sreadingMSFT sreadingMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@sreadingMSFT sreadingMSFT dismissed their stale review October 12, 2021 20:11

revoking review

Copy link
Contributor

@sreadingMSFT sreadingMSFT left a comment

Choose a reason for hiding this comment

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

:shipit:

@florelis florelis requested a review from JohnMcPMS October 12, 2021 23:52
Copy link
Member

@JohnMcPMS JohnMcPMS left a comment

Choose a reason for hiding this comment

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

Minor feedback; looks good.

src/AppInstallerCLICore/Workflows/DownloadFlow.cpp Outdated Show resolved Hide resolved
src/AppInstallerCLICore/Workflows/DownloadFlow.cpp Outdated Show resolved Hide resolved
@florelis florelis merged commit 12dbc22 into microsoft:master Oct 13, 2021
@florelis florelis deleted the multipleDownloads branch October 13, 2021 22:57
@florelis florelis restored the multipleDownloads branch October 13, 2021 23:01
@florelis florelis deleted the multipleDownloads branch October 13, 2021 23:01
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.

4 participants