Implement parallel downloads for COM scenarios #1588
Conversation
| void ContextOrchestrator::AddItemManifestToInstallingSource(const OrchestratorQueueItem& queueItem) | ||
| { | ||
| const auto& manifest = queueItem.GetContext().Get<Execution::Data::Manifest>(); | ||
| m_installingWriteableSource->AddPackageVersion(manifest, std::filesystem::path{ manifest.Id + '.' + manifest.Version }); |
There was a problem hiding this comment.
This is a pre-existing issue, but there is the potential for Id collisions from different sources here. Not an immediate concern but we might want to consider how we can store them all in the same index, or use a separate index per source. Most options are probably annoying and relatively expensive to implement, and the current potential for problems is low. Just leaving this here as a reminder to you and me both.
| { | ||
| ++m_runningThreads; | ||
| std::thread runnerThread(&OrchestratorQueue::RunItems, this); | ||
| runnerThread.detach(); |
There was a problem hiding this comment.
We should probably be keeping the std::thread objects because I don't think that keeping tracking of running threads with a counter will work.
There was a problem hiding this comment.
Basically I think that the use of a counter here is fragile; if the thread terminates abnormally it won't decrement. With proper handling you can make it safer and more robust, but you could also put that effort into using the kernel to know the answer. And it has just a bit more mileage than any code we write here 😄
So I would write a thread wrapper type and hold onto them. You can WaitForSingleObject on the native handle (probably) as suggested https://docs.microsoft.com/en-us/windows/win32/api/processthreadsapi/nf-processthreadsapi-getexitcodethread
That way we could say for a fact how many threads were still running at this point.
ghost
added
the
Needs-Author-Feedback
|
@lechacon this pull request has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 7 days of this comment. |
|
@msftbot, leave the PR open. [Maybe that works?] |
ghost
removed
No-Recent-Activity
This comment has been minimized.
This comment has been minimized.
This changes allows multiple installer downloads to occur in parallel (and parallel to installation) on COM scenarios.
Modified the context orchestrator to have multiple queues for items to execute, each of which will handle a type of command (download & install). Items to execute will move to each queue according to the next command to execute for the item. Each queue handles execution of its items.
Adding a queue item in the orchestrator will add it to the queue corresponding to its first command. Once it is executed, it is moved to the queue for the following command and so on. Each queue knows how many commands of its type can run at the same time (n for download, 1 for install) and how many are currently running. When an item is added, if it has capacity, it starts a thread to run it. And when a thread finishes running a command (after sending it to the orchestrator to re-queue), it runs the next item.
Microsoft Reviewers: Open in CodeFlow