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

Avoid downloading MSBuild SDKs from NuGet on the VS UI thread #6069

Open
dsplaisted opened this issue Apr 7, 2020 · 16 comments
Open

Avoid downloading MSBuild SDKs from NuGet on the VS UI thread #6069

dsplaisted opened this issue Apr 7, 2020 · 16 comments
Assignees
Labels
Milestone

Comments

@dsplaisted
Copy link
Member

The MSBuild NuGetSdkResolver may download NuGet packages during MSBuild evaluation, which can happen on the UI thread in VS. Some discussion of this: dotnet/msbuild#4025

We are adding some features to MSBuild SDK Resolvers to support optional SDK workloads for .NET 5. This will allow SDK resolvers to:

  • Return any number of SDK paths (zero, one, or many). It will be possible to return a successful result that returns no SDK paths.
  • Return MSBuild items and properties to add to the evaluation result

These features will also allow us to improve the NuGetSdkResolver experience in VS. How this could work is outlined in the "NuGet SDK Resolver" section of this designs PR: dotnet/designs#104

The proposal is this: By default, the NuGet SDK resolver would continue to work as it does today. However, in Visual Studio it would be set to a mode which would disable acquisition of the NuGet packages. In this mode, if an SDK NuGet package wasn't already available locally, the resolver would not download the package, but would add a MissingMSBuildSDK item with the name and version of the SDK that needs to be downloaded.

The project system would check for MissingMSBuildSDK items after the project is evaluated. If there are any, the project system would not load the project normally. It would launch an async acquisition process to download the NuGet packages, while showing appropriate UI (for example a spinning progress bar, or "loading..." text by the project in Solution Explorer). When the package acquisition finishes, the project system would reload the project.

We may need to add the ability for resolvers to read global properties in order for VS to communicate to the NuGetSdkResolver that it should run in this different mode.

@davkean
Copy link
Member

davkean commented Apr 7, 2020

I would much prefer the communication to be something as simple as:

  • We get a list of missing packages
  • We pass these to an API (from NuGet) and it downloads it.

We can handle the prompt, etc, but using the project evaluation as side-effect of acquiring the packages would be very hard to coordinate with the solution and the project load life cycle.

@davkean
Copy link
Member

davkean commented Apr 7, 2020

@dsplaisted Given SDKs can refer to other SDKs, how will MSBuild be able to provide a complete set of MissingMSBuildSDK items to avoid the load project -> prompt -> load project -> prompt?

@drewnoakes
Copy link
Member

@davkean might be best for the user if the prompt cleared itself once everything's loaded, regardless of how many downloads occur. The user shouldn't have to interact with the message.

@dsplaisted
Copy link
Member Author

I discussed this with @davkean yesterday. The proposal is that MSBuild would provide a list of SDKs to download. There are some cases where we couldn't know the full dependencies on the first evaluation. This can happen if a NuGet SDK imports another NuGet SDK, or if a NuGet SDK ends up adding a dependency on an optional workload that would need to be installed via in-product acquisition.

As Drew notes, the experience is mitigated if there isn't a prompt the user has to click through in order to download NuGet packages.

Dave suggested that a cheaper first step would be to fail the load in VS if there were unresolved NuGet SDKs, with a message that the project needs to be built from the command line first in order to restore the packages, so that VS can open the project.

@jjmew
Copy link
Contributor

jjmew commented Apr 14, 2020

Triage: @davkean is this something that you should own or should we give it to someone else?

@jjmew
Copy link
Contributor

jjmew commented Apr 21, 2020

I am following up with @marcpopMSFT

@jjmew jjmew added this to the 16.8 milestone May 15, 2020
@jjmew jjmew added the Triage-Approved Reviewed and prioritized label May 15, 2020
@jjmew
Copy link
Contributor

jjmew commented May 15, 2020

The idea is that we will block it and ask the user to use the command line to restore. Later on we might do something beyond that.

@jjmew
Copy link
Contributor

jjmew commented Oct 6, 2020

@swesonga Could you work on this for 16.9?

@marcpopMSFT
Copy link
Member

If there is only time for the error message in 16.9, we should collect some feedback from internal folks so I started a mail thread. That way we can determine if customers would rather a UI hang versus an error message and command line restore. @KathleenDollard for feedback as well.

@jjmew jjmew added the Must-Have Items that must be delivered by the end of the assigned milestone label Oct 13, 2020
@swesonga swesonga added the Bug Bash Issues to be handled during our bug fixing push label Nov 25, 2020
@jjmew jjmew modified the milestones: 16.9, 16.10 Feb 9, 2021
@jjmew jjmew modified the milestones: 16.10, 17.0 Jun 22, 2021
@CyrusNajmabadi
Copy link
Member

Note: this full on hung VS for me, forcing me to force-quit it.

@jjmew jjmew modified the milestones: 17.0, 17.1 Sep 20, 2021
@aortiz-msft
Copy link

@melytc - Please feel free to sync with @jeffkl on this item.

@melytc melytc removed Must-Have Items that must be delivered by the end of the assigned milestone Bug Bash Issues to be handled during our bug fixing push labels Mar 4, 2022
@melytc melytc modified the milestones: 17.1, 17.3 Mar 4, 2022
@marcpopMSFT
Copy link
Member

Per @jeffkl and @melytc many of the worst problems have been improved here and Jeff continues to make improvements. We're still going to leave this active to consider moving the download off of the UI thread but it may not be as critical anymore.

@davkean
Copy link
Member

davkean commented Mar 16, 2022

@marcpopMSFT Evaluation is already done off the UI thread in CPS, so moving the download off the UI thread does not resolve the problem. However, VS in lots of circumstances, needs to block on evaluation from the UI thread and that evaluation hitting the network is the problem.

@jeffkl How have you determined the worst problems have been improved here?

@jeffkl
Copy link
Contributor

jeffkl commented Mar 17, 2022

How have you determined the worst problems have been improved here?

I have made some big improvements around the whole scenario. There's a lot more going on than just downloading a package. For each design-time build, a global.json is searched for and deserialized, NuGet.Config is loaded, then the disk is searched for an existing package, then finally a package is downloaded. Also, MSBuild's SDK resolution APIs were launching a lot of threads to handle out-of-proc design-time builds (evaluations) in the main node which could lead to thread starvation. I also moved the NuGet-based MSBuild project SDK resolver down in priority so that the .NET SDK resolver runs first in case its just a built in SDK being resolved.

I have more work to do in this area so don't think I'm done improving things.

NuGet/Home#11441

@davkean
Copy link
Member

davkean commented Mar 17, 2022

I've seen that, but most traces I've seen around this are caused by the network hit. For example, for the original bug against msbuild, the symptoms that led me to file was that the corefx solution was taking 15 minutes to open - what data are we using to determine that the underlying problem is resolved?

@jeffkl
Copy link
Contributor

jeffkl commented Mar 17, 2022

Did it take 15 minutes to download a package? I'm assuming that there was some thread starvation going on where a ton of threads were all trying to resolve an SDK and it took a lot longer to get a nupkg.

I have never been able to reproduce the issue. I've generated a large solution that references five different SDKs. I've injected test SDK resolvers that have sleeps, and I've tried clone repos that exhibit the problems but I have never been able to get it to happen live for me to debug.

I am not making wild claims that its all fixed. I have been making changes to the whole stack that should improve the experience that should make a difference. Do you have time to help me get some data that I can use to make meaningful comparisons?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests