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

Split COM install command into download and install stages #1528

Merged
merged 14 commits into from
Oct 13, 2021
Merged

Split COM install command into download and install stages #1528

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a3780d3
Sample change for multiple concurrent downloads.
sreadingMSFT Sep 4, 2021
2c120b5
Add hash re-check when file already exists
Sep 29, 2021
ef62a7f
Skip removing motw if not present
Sep 29, 2021
1e9df7d
Generate installer path only once
Sep 29, 2021
07d6b75
Update comment
Sep 29, 2021
8e7641a
Spelling
Sep 30, 2021
5d93b69
Comments from PR
Oct 8, 2021
24d3dfa
Merge branch 'master' into multipleDownloads
Oct 8, 2021
3f92091
Remove outdated comment
Oct 8, 2021
92822a6
Check if file exists when no motw found
Oct 11, 2021
b4a3ab4
Add back rename downloaded file
Oct 11, 2021
6acf52f
Fix failing test
Oct 11, 2021
918e4f2
Fix tests
Oct 12, 2021
4f0886e
PR comments
Oct 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .github/actions/spelling/allow.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,6 +385,7 @@ regex
regexp
removemanifest
repolibtest
requeue
rescap
resheader
resmimetype
Expand Down
2 changes: 2 additions & 0 deletions src/AppInstallerCLICore/AppInstallerCLICore.vcxproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,6 +275,7 @@
<ClInclude Include="VTSupport.h" />
<ClInclude Include="PackageCollection.h" />
<ClInclude Include="Workflows\CompletionFlow.h" />
<ClInclude Include="Workflows\DownloadFlow.h" />
<ClInclude Include="Workflows\ImportExportFlow.h" />
<ClInclude Include="Workflows\MsiInstallFlow.h" />
<ClInclude Include="Workflows\MSStoreInstallerHandler.h" />
Expand Down Expand Up @@ -324,6 +325,7 @@
<ClCompile Include="Resources.cpp" />
<ClCompile Include="VTSupport.cpp" />
<ClCompile Include="Workflows\CompletionFlow.cpp" />
<ClCompile Include="Workflows\DownloadFlow.cpp" />
<ClCompile Include="Workflows\ImportExportFlow.cpp" />
<ClCompile Include="Workflows\MsiInstallFlow.cpp" />
<ClCompile Include="Workflows\MSStoreInstallerHandler.cpp" />
Expand Down
6 changes: 6 additions & 0 deletions src/AppInstallerCLICore/AppInstallerCLICore.vcxproj.filters
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,9 @@
<ClInclude Include="Workflows\SettingsFlow.h">
<Filter>Workflows</Filter>
</ClInclude>
<ClInclude Include="Workflows\DownloadFlow.h">
<Filter>Workflows</Filter>
</ClInclude>
</ItemGroup>
<ItemGroup>
<ClCompile Include="pch.cpp">
Expand Down Expand Up @@ -307,6 +310,9 @@
<ClCompile Include="Workflows\SettingsFlow.cpp">
<Filter>Workflows</Filter>
</ClCompile>
<ClCompile Include="Workflows\DlownloadFlow.cpp">
<Filter>Workflows</Filter>
</ClCompile>
</ItemGroup>
<ItemGroup>
<None Include="PropertySheet.props" />
Expand Down
14 changes: 12 additions & 2 deletions src/AppInstallerCLICore/Commands/COMInstallCommand.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// Licensed under the MIT License.
#include "pch.h"
#include "COMInstallCommand.h"
#include "Workflows/DownloadFlow.h"
#include "Workflows/InstallFlow.h"
#include "Workflows/WorkflowBase.h"

Expand All @@ -13,12 +14,21 @@ using namespace AppInstaller::Utility::literals;
namespace AppInstaller::CLI
{
// IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data
void COMInstallCommand::ExecuteInternal(Context& context) const
void COMDownloadCommand::ExecuteInternal(Context& context) const
{
context <<
Workflow::ReportExecutionStage(ExecutionStage::Discovery) <<
Workflow::SelectInstaller <<
Workflow::EnsureApplicableInstaller <<
Workflow::InstallSinglePackage;
Workflow::DownloadSinglePackage;
}

// IMPORTANT: To use this command, the caller should have already executed the COMDownloadCommand
void COMInstallCommand::ExecuteInternal(Context& context) const
{
context <<
Workflow::GetInstallerHash <<
Workflow::VerifyInstallerHash <<
Workflow::InstallPackageInstaller;
florelis marked this conversation as resolved.
Show resolved Hide resolved
}
}
9 changes: 9 additions & 0 deletions src/AppInstallerCLICore/Commands/COMInstallCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,15 @@

namespace AppInstaller::CLI
{
// IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data
struct COMDownloadCommand final : public Command
{
COMDownloadCommand(std::string_view parent) : Command("download", parent) {}

protected:
void ExecuteInternal(Execution::Context& context) const override;
};

// IMPORTANT: To use this command, the caller should have already retrieved the package manifest (GetManifest()) and added it to the Context Data
struct COMInstallCommand final : public Command
{
Expand Down
4 changes: 3 additions & 1 deletion src/AppInstallerCLICore/Commands/RootCommand.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,9 @@ namespace AppInstaller::CLI
{
struct RootCommand final : public Command
{
RootCommand() : Command("root", {}) {}
constexpr static std::string_view CommandName = "root"sv;

RootCommand() : Command(CommandName, {}) {}

std::vector<std::unique_ptr<Command>> GetCommands() const override;
std::vector<Argument> GetArguments() const override;
Expand Down
26 changes: 22 additions & 4 deletions src/AppInstallerCLICore/ContextOrchestrator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,13 @@ namespace AppInstaller::CLI::Execution
}
}

void ContextOrchestrator::RequeueItem(OrchestratorQueueItem& item)
{
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.

}

void ContextOrchestrator::EnqueueAndRunItem(std::shared_ptr<OrchestratorQueueItem> item)
{
EnqueueItem(item);
Expand Down Expand Up @@ -101,11 +108,10 @@ namespace AppInstaller::CLI::Execution
HRESULT terminationHR = S_OK;
try
{
::AppInstaller::CLI::RootCommand rootCommand;
std::unique_ptr<Command> command = item->PopNextCommand();

std::unique_ptr<AppInstaller::ThreadLocalStorage::PreviousThreadGlobals> setThreadGlobalsToPreviousState = item->GetContext().GetThreadGlobals().SetForCurrentThread();

std::unique_ptr<::AppInstaller::CLI::Command> command = std::make_unique<::AppInstaller::CLI::COMInstallCommand>(rootCommand.Name());
item->GetContext().GetThreadGlobals().GetTelemetryLogger().LogCommand(command->FullName());
command->ValidateArguments(item->GetContext().Args);

Expand All @@ -123,7 +129,16 @@ namespace AppInstaller::CLI::Execution
item->GetContext().SetTerminationHR(terminationHR);
}

RemoveItemInState(*item, OrchestratorQueueItemState::Running);
item->GetContext().EnableCtrlHandler(false);

if (FAILED(terminationHR) || item->IsComplete())
{
RemoveItemInState(*item, OrchestratorQueueItemState::Running);
}
else
{
RequeueItem(*item);
}

item = GetNextItem();
}
Expand Down Expand Up @@ -180,7 +195,10 @@ namespace AppInstaller::CLI::Execution

std::unique_ptr<OrchestratorQueueItem> OrchestratorQueueItemFactory::CreateItemForInstall(std::wstring packageId, std::wstring sourceId, std::unique_ptr<COMContext> context)
{
return std::make_unique<OrchestratorQueueItem>(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context));
std::unique_ptr<OrchestratorQueueItem> item = std::make_unique<OrchestratorQueueItem>(OrchestratorQueueItemId(std::move(packageId), std::move(sourceId)), std::move(context));
item->AddCommand(std::make_unique<::AppInstaller::CLI::COMDownloadCommand>(RootCommand::CommandName));
item->AddCommand(std::make_unique<::AppInstaller::CLI::COMInstallCommand>(RootCommand::CommandName));
return item;
}

}
11 changes: 11 additions & 0 deletions src/AppInstallerCLICore/ContextOrchestrator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
#include "ExecutionArgs.h"
#include "ExecutionContextData.h"
#include "CompletionData.h"
#include "Command.h"
#include "COMContext.h"

#include <string_view>
Expand Down Expand Up @@ -40,11 +41,20 @@ namespace AppInstaller::CLI::Execution
COMContext& GetContext() const { return *m_context; }
const wil::unique_event& GetCompletedEvent() const { return m_completedEvent; }
const OrchestratorQueueItemId& GetId() const { return m_id; }
void AddCommand(std::unique_ptr<Command> command) { m_commands.push_back(std::move(command)); }
std::unique_ptr<Command> PopNextCommand()
{
std::unique_ptr<Command> command = std::move(m_commands.front());
m_commands.pop_front();
return command;
}
bool IsComplete() const { return m_commands.empty(); }
private:
OrchestratorQueueItemState m_state = OrchestratorQueueItemState::NotQueued;
std::unique_ptr<COMContext> m_context;
wil::unique_event m_completedEvent{ wil::EventOptions::ManualReset };
OrchestratorQueueItemId m_id;
std::deque<std::unique_ptr<Command>> m_commands;
};

struct OrchestratorQueueItemFactory
Expand All @@ -67,6 +77,7 @@ namespace AppInstaller::CLI::Execution
void RunItems();
std::shared_ptr<OrchestratorQueueItem> GetNextItem();
void EnqueueItem(std::shared_ptr<OrchestratorQueueItem> item);
void RequeueItem(OrchestratorQueueItem& item);
void RemoveItemInState(const OrchestratorQueueItem& item, OrchestratorQueueItemState state);

_Requires_lock_held_(m_queueLock)
Expand Down
Loading