NuGet should not eagerly start restore when there are pending design time builds

[nkolev92]

Experience tracked: 1355522

A couple of real time scenarios here:

• A user edited a common file such as Directory.Build.Props
• Git operations such as branch switch.

NuGet has an easy tell about when to restore on solution load (wait for all the projects to be ready), during branch switching, that's not obvious because most of the projects are already ready.

This issue is blocked on 1283992

[nkolev92]

There's a design in flight for this:

// a new service defined inside NuGet to align with current services.

public interface IVsSolutionRestoreService4

{

                // Project system calls NuGet service (optionally) to register itself.

                // Each project can only register no more than one time

                // when project is unloaded, it need dispose the registration object.

                IDisposable RegisterRestoreInfoSource(string projectUniqueName, IVsProjectRestoreInfoSource restoreInfoSource);

}

 

 

// implemented by the project system, and is called by NuGet to know whether there is any pending restore work

// so it can hold the current batch.

public interface IVsProjectRestoreInfoSource

{

                // true means the project system plans to call NominateProjectAsync in the future.

                bool HasPendingRestore();

 

                // NuGet calls this method to wait project to nominate restoring

                // If the project has no pending restore data, it will return a completed task.

                // Otherwise a task which will be completed once the project norminate the next restore

                // the task will be cancelled, if the project system decide it no longer need restore (for example: the restore state has no change)

                // the task will be failed, if the project system runs into a problem, so it cannot get correct data to norminate a restore (DT build failed)

                Task WhenRestoreNominated();

}

[lifengl]

After thinking about this again, I now prefer a slightly different version of it:

The difference:
instead of each project registers itself, it can be done by providing one component for one type of project systems (depending on the implementation of the project system, one can be more efficient or easier to do that another one. Also, potentially, other features like a batch file changes might use it to block restore as well. Basically, it decouple a source from a single project, and NuGet doesn't need manage projects by name as well.

The drawback: if NuGet wants to use the state based on a subset of projects. For example, if it decides a project which is not depending on any project touched by the restore operation & being referenced by the set, it might ignore the pending state of a specific project. Although this is quite complex, and also p2p references are dynamic, so i feel the second option sounds better.

Here is the alternative version of it:

    public interface IVsSolutionRestoreService4
    {
        // Project system calls NuGet service (optionally) to register itself.
        // Each project can only register no more than one time
        // when project is unloaded, it need dispose the registration object.
        IDisposable RegisterRestoreInfoSource(IVsProjectRestoreInfoSource restoreInfoSource);
    }

    // implemented by the project system, and is called by NuGet to know whether there is any pending restore work
    // so it can hold the current batch.
    public interface IVsProjectRestoreInfoSource
    {
        /// <summary>
        /// Gets a name which can be used in diagnostic logs
        /// </summary>
        string Name { get; }

        // true means the project system plans to call NominateProjectAsync in the future.
        bool HasPendingRestore();

        // NuGet calls this method to wait project to nominate restoring
        // If the project has no pending restore data, it will return a completed task.
        // Otherwise a task which will be completed once the project norminate the next restore
        // the task will be cancelled, if the project system decide it no longer need restore (for example: the restore state has no change)
        // the task will be failed, if the project system runs into a problem, so it cannot get correct data to norminate a restore (DT build failed)
        Task WhenRestoreNominated();
    }

[zivkan]

Firstly, I think HasPendingRestore() should be HasPendingNomination(). Maybe this was a typo.

However, I think that bool HasPendingRestore() is possibly not needed. Perhaps there's a perf benefit if WhenRestoreNominated() is allowed to return null? But given on project load I imagine you'd set the project to the "has pending restore" state, it effectively means there will never be a scenario when a TaskCompletionSource is not allocated. If you keep the instance in memory in a completed state until the next time nomination is required, then I'm not aware of any perf impact having only the method returning the Task.

!WhenRestoreNominated().IsCompleted == HasPendingRestore(), WhenRestoreNominated().IsFaulted means the project is in a broken state (NuGet can skip restoring this project, if there was a previously successful nomination, and skip restoring all other projects that have a p2p reference to this broken project?), and WhenRestoreNominated().IsCompletedSuccessfully is when NuGet should restore this project.

I'm not sure that putting the task in a cancelled state when the project has no changes is very beneficial, as NuGet can realize that we didn't get a nomination for the project. Nikolche knows VS restore better than me, so he can correct me, but I imagine we'll treat WhenRestoreNominated().IsCancelled the same as IsCompletedSuccessfully. Our no-op restore logic doesn't depend on the project system telling us nothing changed, although maybe Nikolche may have better ideas. I think a DateTime LastNominationTime in our internal data structures would be functionally equivalent, and allows NuGet to infer the no-op state based on information it received and reduces the risk of bugs where the project system sent a nomination, but then cancelled the task.

[lifengl]

Thanks,

HasPendingNomination sounds better, and I agree with you that it is not essential. I also thought about removing it, but ended with keeping it due to:
1, it was brought up in the original discussion, so maybe it is useful for certain scenarios where the caller doesn't need wait. (Like logging?)
2, it is easier to understand than using the state of the task returned from this WhenRestoreNorminated
3, I am not exactly sure whether WhenRestoreNorminated need do extra work in different implementation, because it may need chain events to watch thing to happen, the other one just need check the current state.

On the other hand, maybe NuGet doesn't need this boolean check at all, so I would like to hear how others think about it.

Original thought is that the task will be cancelled, if the project is closed or have anther reason that the nomination is not needed. But I guess your point might be right, from NuGet point of view, if it just need a signal to stop waiting, it might be the same as a completed task. If we pick up that, maybe consider adjusting the name of this method, because really nothing has been nominated, otherwise, it might cause some confusion. Other than that, the difference between the two might be slightly useful for logging, but just like you mentioned, your internal data structure might have the state, so I am fine with both.

[nkolev92]

I think the registration mechanism should use the same unique id as the regular nomination, so the name isn't optional imo, but necessary.
That way NuGet can correctly handle load/unload project events, otherwise we're need to talk about a deregistration mechanism.

I agree that the task status doesn't matter to NuGet at all.

I see the need for both method for reporting purposes.

This has the potential to delay restore for a while, so logging at a high verbosity at some intervals might be a good idea.
NuGet would likely call these methods quite frequently, so maybe we can avoid allocations by calling HasPendingNominations first and then calling the WhenNominated on the ones that return true?
When whatever task(s) all return, NuGet would probably need to go over all projects yet again.

Does that make sense for both of you?
Would you have expected NuGet to do something different?

@zivkan The timestamp idea is interesting, but it has source of truth challenges, because nomination time is owned by NuGet, but the service is implemented by the project system. Am I missing something? I'd be happy to chat offline.

[nkolev92]

If we want to be pedantic, we can probably call it WhenDesignTimeBuildCompleted() and not really a restore nomination.

I think both me and @zivkan are suggesting that the following is enough.

        // NuGet calls this method to wait project to nominate restoring
        // If the project has no pending restore data, it will return a completed task.
        // Otherwise a task which will be completed once the project norminate the next restore
        Task WhenRestoreNominated();

[nkolev92]

Status update:

I'm playing with the proposed API shape in https://github.com/NuGet/NuGet.Client/compare/dev-nkolev92-solutionRestoreService4?expand=1.

I have some small changes, but nothing major.

Note that the implementation on NuGet side is not super critical, it's mostly for me to be able to understand the complete process.

The changes will go into 2 PRs, even if they are implement right now.

1. Add the API and expose the new service.
2. Actually implement the algorithm to wait for restore longer.

Here's my WIP right now, hoping to have a more concrete proposal in the next few days.

namespace NuGet.SolutionRestoreManager
{
    /// <summary>
    /// Represents a package restore service API for integration with a project system.
    /// </summary>
    [ComImport]
    [Guid("72327117-6552-4685-BD7E-9C40A04B6EE5")]
    public interface IVsSolutionRestoreService4
    {
        /// <summary>
        /// A project system can call this service (optionally) to register itself.
        /// Each project can only register once.
        /// NuGet will remove the registered object when the project is unloaded.
        /// </summary>
        /// <param name="restoreInfoSource"></param>
        /// <param name="cancellationToken"></param>
        public Task RegisterRestoreInfoSourceAsync(IVsProjectRestoreInfoSource restoreInfoSource, CancellationToken cancellationToken);
    }
    
    [ComImport]
    [Guid("35AD5FF2-6AB7-48E9-BCC0-189042410FA6")]
    public interface IVsProjectRestoreInfoSource
    {
        /// <summary>
        /// Project Unique Name.
        /// Must be equivalent to the name provided in the <see cref="IVsSolutionRestoreService3.NominateProjectAsync(string, IVsProjectRestoreInfo2, System.Threading.CancellationToken)"/> or equivalent.
        /// </summary>
        /// <remarks>Never <see langword="null"/>.</remarks>
        string Name { get; }

        /// <summary>
        /// Whether the source needs to do some work that could lead to a nomination.
        /// This method may be called frequently, so it should be very efficient.
        /// </summary> 
        bool HasPendingNomination();

        /// <summary>
        /// NuGet calls this method to wait project to nominate restoring.
        /// If the project has no pending restore data, it will return a completed task.
        /// Otherwise a task which will be completed once the project norminate the next restore
        /// </summary>
        Task WhenNominated(CancellationToken cancellationToken);
    }
}

In particular the changes are:

• Adding cancellation tokens where appropriate.
• Asyncifying the registration method.
• Making the WhenNominated status redudant. Alternatively, it could be implement, but NuGet won't observe it now. If at any point we decide that this status is useful, it'll be there for free.
• Removing the disposable object. NuGet stores all information in a cache, so it should be able to clear the cache when a project gets unloaded.

Any concerns with this direction @lifengl @zivkan @ocalesp @drewnoakes

[lifengl]

That sounds good, and I like passing in a CancellationToken to WhenNominated method. I do think you might need it one day. To make sure we are on the same page:
1, when the project system runs into some problems and the design time build failed -- which means it cannot nominate NuGet restore work, the task from WhenNominated will fail, and allow you to skip it. When you are using HasPendingNomination, you will have to call it repeatedly.

2, for RegisterRestoreInfoSourceAsync:
First, the project system will not wait on it during loading the solution. Maybe it should wait on that before closing the project. We don't want it to slow down loading the solution, as it is not essential to us.
I noticed your implementation need UI thread, waiting on anything blocking on UI thread during project loading time will lead a regression for us. It has a potential issue: if the task to register a project is blocked due to UI thread contention, you may not use it to block a pending restore work. Not sure whether it is a real issue, I personally prefer to find a UI thread free, low contention implementation.

3, it is fine to use solution events to clean up registered projects, I personally feel might leave gaps in some special situations (like creating a project but aborted, or the solution open is aborted quickly, and the registration task may finish when solution is closing. But maybe it is fine if the project system pass in CancellationToken correctly, and blocking on it before close, so maybe it is not a problem in the real world, just watch for it during implementation & testing.

Note: when I read your implementation, it might run into problems in some special situation like creating a project. Basically, RegisterRestoreInfoSource will be called much earlier than NominateProject will be called. That is the intention that the project should not wait design time build before registering it (that is the reason why we need it, to give you a hint the project may want to restore one day, we do it as early as possible). NominateProject won't happen until DT build finishes, so it will never happen until a project is added to the solution, but the RegisterRestoreInfo doesn't align with the solution state, it is coming from the project during very early initialization stage, so it can be a problem if your code makes some assumptions between projects and the solution.

[nkolev92]

1, when the project system runs into some problems and the design time build failed -- which means it cannot nominate NuGet restore work, the task from WhenNominated will fail, and allow you to skip it. When you are using HasPendingNomination, you will have to call it repeatedly.

Ah good point. I missed that.
Currently we don't account for that, and we haven't had too many issues around it.
I get your point though. This could allow us to do better. I think we can add it back then, even if NuGet ignores it in the short term.

2, for RegisterRestoreInfoSourceAsync:
First, the project system will not wait on it during loading the solution. Maybe it should wait on that before closing the project. We don't want it to slow down loading the solution, as it is not essential to us.
I noticed your implementation need UI thread, waiting on anything blocking on UI thread during project loading time will lead a regression for us. It has a potential issue: if the task to register a project is blocked due to UI thread contention, you may not use it to block a pending restore work. Not sure whether it is a real issue, I personally prefer to find a UI thread free, low contention implementation.

The UI thread transition should happen infrequently, only when the project is completely new to NuGet.
I don't think it's too big of a problem if a registration is delayed due to UI contention. At worst, it's what we have today.

3, it is fine to use solution events to clean up registered projects, I personally feel might leave gaps in some special situations (like creating a project but aborted, or the solution open is aborted quickly, and the registration task may finish when solution is closing. But maybe it is fine if the project system pass in CancellationToken correctly, and blocking on it before close, so maybe it is not a problem in the real world, just watch for it during implementation & testing.

Great point. We can watch for that.

but the RegisterRestoreInfo doesn't align with the solution state, it is coming from the project during very early initialization stage, so it can be a problem if your code makes some assumptions between projects and the solution.

Are you suggesting that NuGet using IVsSolution to initialize project details won't work?

I'll try to clean-up the implementation again. I'll probably do a separate PR to get the contract merged first so that we can unblock the work on project system side.

[lifengl]

NuGet can use IVsSolution to initialize project details, but it is possible that RegisterRestoreInfoSourceAsync is called earlier than the solution knows the project. However, the project won't nominate NuGet restoration until it is added to the solution. It is tricky part because the project system and the solution is mostly decoupled. When we load a solution, of course, the solution knows the project before it is loaded. However, when a new project is created, the project will be loaded, then the solution gather information from the project, and adds that information to itself. During the creation phase, it is possible RegisterRestoreInfoSourceAsync will be called much earlier than you expect. However, project won't start to restore NuGet packages until it finishes DT builds, and DT builds won't start until the project is added to the solution, which is essential to resolve P2P references. That dependency is largely outsight of the project system, the design time build system is responsible to align that. In the majority of the project side code, it doesn't care about the solution, especially the solution provides only UI thread COM API, which can lead performance problems for us.

If you assumes the project is in the solution when RegisterRestoreInfoSourceAsync is called, it may be tricky in some project creation/adding scenarios, and may expose some race conditions. Anyway, this is more implementation detail, and we can keep the contract as it is to move ahead.

And for UI thread transition delay, if it is an issue, we can always hold NuGet restore until all on-going RegisterRestoreInfoSourceAsync tasks to complete, just the way it waits a virtual project. In any case, it is easily resolvable problem, so I don't have any problem with the current design, and I think we can go ahead to start the implementation.

[nkolev92]

@lifengl

The API in NuGet/NuGet.Client#4058 is ready for review.

I'm just defining the API right now, so that project-system can start their implementation.

[nkolev92]

Marking as blocked on dotnet/project-system#7199 for now, which in turn is blocked on a nuget insertion into main.

[nkolev92]

NuGet side has been merged into main.