-
Notifications
You must be signed in to change notification settings - Fork 337
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
Allow users to download multiple VM images at the same time from the Hyper-V gallery #3687
Allow users to download multiple VM images at the same time from the Hyper-V gallery #3687
Conversation
extensions/HyperVExtension/src/HyperVExtension/Models/ByteTransferProgress.cs
Outdated
Show resolved
Hide resolved
...ons/HyperVExtension/src/HyperVExtension/Models/VirtualMachineCreation/FileDownloadMonitor.cs
Outdated
Show resolved
Hide resolved
...ons/HyperVExtension/src/HyperVExtension/Models/VirtualMachineCreation/FileDownloadMonitor.cs
Outdated
Show resolved
Hide resolved
...ons/HyperVExtension/src/HyperVExtension/Models/VirtualMachineCreation/FileDownloadMonitor.cs
Outdated
Show resolved
Hide resolved
if (!HasDownloadCompleted()) | ||
{ | ||
// Wait for the last download to complete before moving on. | ||
_waitForDownloadCompletion.WaitOne(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should think about adding a way (and UX) to cancel this download. It can get stuck forever.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I thought about adding a timeout, but I figure it depends on the users internet connection so I can't really say for sure how long it'll take. I'll make a GitHub issue for this to update the UI. We need to get the cancellation token from the IAsyncOperation<CreateComputeSystemResult> StartAsync()
method above and use it here to cancel the operation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good news, looks like I actually originally do send a cancelation request from Dev Home to the extension here:
CreateComputeSystemResult = await _createComputeSystemOperation.StartAsync().AsTask(_cancellationTokenSource.Token); |
So I have updated the code to allow for cancellation: See below video as we now only use async/awaits:
Video.of.me.showing.that.you.can.now.cancel.downloads.from.Dev.Home.for.Hyper-V.mp4
As you can see we can cancel them now. We'll just need to add a content dialog to ask the user if they're sure.
lock (_lock) | ||
{ | ||
if (_destinationFileDownloadMap.TryGetValue(destinationFilePath, out var subscribers)) | ||
{ | ||
return true; | ||
} | ||
|
||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In StartDownloadAsync this lock is taken for the duration of the downloading process, so this method won't return until downloading is done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From what I see I don't see how that would be the case. I don't lock the entire StartDownloadAsync method. From my testing, this doesn't get locked for the entirety of the StartDownloadAsync call. But please let me know if I'm missing something. Thanks.
Edit, While my first comment still stands as I'd appreciate some insights in case I missed something. I moved the first lock outside the try/catch in StartDownloadAsync, so the finally block doesn't inadvertently remove the monitor from the map after subsequent download requests for the same file.
Here is me creating 6 VMs using the same image consecutively with no errors. Note that all of them share the same download progress.
Summary of the pull request
There are two steps in creating a VM from the VM gallery that involves a file.
Currently when users attempt to create a VM multiple times simultaneously, this error is thrown: "The process cannot access the file because it is being used by another process. The file is in use. Please close the file before continuing". This is thrown during the download portion. To prevent this from happening, we can reuse the download progress for subsequent download requests of the same file after one has been initiated.
Changes in the Hyper-V extension:
FileDownloadMonitor
class that is used to monitor the progress of a download. Clients can subscribe to the monitor who will publish the progress of the download as it receives them to these subscribers.DownloaderService
to use theFileDownloadMonitor
to start the download and add clients to the monitor based on the absolute file path of the file that will be downloaded.DownloaderService
class to allow for two possibilities. One where it handles the case where a new file is requested to be downloaded and two where it handles the case where the requested file is already being downloaded.VMGalleryVMCreationOperation
's StartAsync's method fails.Here is a video of me attempting to create two of the same VMs BEFORE this change:
Video.showing.file.in.use.error.when.multiple.downloads.are.in.progress.mp4
Here is a video of me attempting to create two of the same VMs AFTER this change:
Timestamps:
Video.showing.me.being.able.to.create.two.Hyper-V.VMs.at.the.same.time.mp4
Added cancelation from the Dev Home side:
Video.of.me.showing.that.you.can.now.cancel.downloads.from.Dev.Home.for.Hyper-V.mp4
References and relevant issues
Note: I did think about using the
AsyncManualResetEvent
to allow threads waiting for the download to complete to wait asynchronously. See the nuget dependency here: https://www.nuget.org/packages/Microsoft.VisualStudio.Threading/. But adding this dependency causes a lot of async errors and warnings (via Microsoft.VisualStudio.Threading.Analyzers) with regards to Dev Home's usage of certain async patterns. So instead I used the WaitAsync method of the SemaphoreSlim class. In the future we should look into that library as it has some good async functionality.Detailed description of the pull request / Additional comments
Validation steps performed
PR checklist