Do not clone objects in DependencyGraphSpec during command-line restore #5645
Conversation
nkolev92
left a comment
nkolev92
left a comment
There was a problem hiding this comment.
I'm comfortable with changing the behavior of the API altogether if it's only called from the CLI.
fwiw, I really thought we were already doing this in both static graph and standard restore.
| /// Creates a <see cref="DependencyGraphSpec" /> from the specified MSBuild items. | ||
| /// </summary> | ||
| /// <param name="items">An <see cref="IEnumerable{T}" /> of <see cref="IMSBuildItem" /> objects representing the MSBuild items gathered for restore.</param> | ||
| public static DependencyGraphSpec GetDependencySpec(IEnumerable<IMSBuildItem> items) |
There was a problem hiding this comment.
Any reason not to make this the default?
It's only used in CLI scenarios right?
It's VS where mutation is a concern correct?
There was a problem hiding this comment.
It's not clear to me why VS would need to mutate a DGSpec. If projects are loaded/unloaded, a new DGSpec can be created with the new project list.
There was a problem hiding this comment.
I would also like a better understanding of why the items in the DG spec are cloned?
There was a problem hiding this comment.
The readonly part can be a bit misleading.
Readonly means that the PackageSpec is not cloned.
The primary reason around package spec cloning is defensiveness. In Visual Studio as a long lived process, the PackageSpec is not guaranteed to be immutable.
After this, we're down to 2 clones, 1 in the RestoreRunner, 1 in the CPSPackageReferenceProject, the latter one is needed, but the previous I'm 99% sure we don't need anymore.
Long term the approach should be;
- Introduce a PackageSpec lock mechanism, and the dg spec would only accept locked PackageSpec. Then it can just avoid cloning.
|
@jeffkl Should this "close" the issue it's linked? Given that we're not fixing the VS side, I think that issue should remain open. |
I almost opened a new issue but hesitated because I don't think we'll ever "fix" the Visual Studio side of things since it needs to mutate the dgspec right? The person who opened the original issue really only cares about command-line based restore at the moment so I figured this pull request would fix the issue and if we wanted we could open a new one for Visual Studio restores? Or I can open a new one for command-line based restores? I do not have a strong opinion. |
|
I've opened NuGet/Home#13270 as a way of tracking the removal of the cloning in Visual Studio code paths. |
Bug
Fixes: NuGet/Home#9041
Regression? Last working version:
Description
@KirillOsenkov recently helped us out and mentioned that the linked issue is of great importance to his current efforts. This change specifies the
readOnlyflag to theDependencyGraphSpecconstructor indicating that the objects it contains do not need to be cloned. I added this constructor for static graph-based restore but didn't add it to regular restore so this change makes them the same.I added an overload to prevent a breaking API change even though I doubt anyone would be affected. The existing constructor just keeps the existing behavior.
PR Checklist
PR has a meaningful title
PR has a linked issue.
Described changes
Tests
Documentation