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

Remove or improve configuration change events #8479

Open
zivkan opened this issue Aug 21, 2019 · 8 comments
Open

Remove or improve configuration change events #8479

zivkan opened this issue Aug 21, 2019 · 8 comments
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:SDK The NuGet client packages published to nuget.org Priority:2 Issues for the current backlog. Product:VS.Client Type:Bug

Comments

@zivkan
Copy link
Member

zivkan commented Aug 21, 2019

ISettings and IPackageSourceProvider have events about changes to settings. However, the only place this appears to be used is in ExtensibleSourceRespositoryProvider and the only place it appears to be raised is in VSSettings when solutions are opened or closed.

The easier option would be to make ExtensibleSourceRespositoryProvider subscribe to the solution open and close events directly and remove all the configuration event code.

Alternatively, events should be unsubscribed properly, probably by making everything related IDisposable and making sure they get disposed correctly.

@sharwell
Copy link

I reached this issue via the [Obsolete] attribute applied to the SourceRepositoryProvider constructor, but neither the attribute nor this issue explain what I should be doing instead.

@zivkan
Copy link
Member Author

zivkan commented Oct 30, 2019

If you're using it in a process that is short lived, use the overload without the enablePackageSourcesChangedEvent argument (the constructor that is not marked as obsolete), as memory leaks from not unsubscribing from events won't affect the process.

If you're using in a long running process, then it's best to use this constructor which is marked as obsolesce and be prepared that in future versions of NuGet there's an increased risk of breaking changes.

sharwell added a commit to sharwell/roslyn-sdk that referenced this issue Oct 31, 2019
@JakeSays
Copy link

JakeSays commented Mar 4, 2020

@zivkan If the idea is to disable package source changed events then none of the non-obsolete ctors support that scenario. The only option takes a IPackageSourceProvider, and the only non-obsolete ctors on PackageSourceProvider enable source changed events thus causing the leak.

So if I am understanding things correctly we really don't have a way to disable events without using an obsoleted ctor. Is this correct?

@zivkan
Copy link
Member Author

zivkan commented Mar 4, 2020

@JakeSays yes, you're right. If you have a long-lived process where you need to create PackageSourceProvider, but you don't want it to subscribe to the solution events to avoid memory leaks, you need to use the ctor marked as obsolete. If your process is short lived, or you keep the PackageSourceProvider around for most of the processes lifetime, then it really doesn't matter. Also, if you let your settings object get garbage collected, it also doesn't matter. However, in Visual Studio, NuGet keeps a setting object for as long as the solution is open. It's been over 6 months, so I might have forgotten the details, but from memory the only scenario where this matters is when you keep the settings object for a long time, but create short lived package source provider objects.

Unfortunately I was between a rock and a hard place. We didn't want to break API & ABI compatibility when making the change, so removing the "useless" events was ruled out. But, in the long term the new ctor will not be needed, the extra parameter would do nothing, so I didn't want people depending on it and therefore getting a breaking change when we finally get rid of it.

I was constrained by previous design decisions and a fear of breaking changes, so for better or worse, this is the decision I made.

@nkolev92 nkolev92 added Priority:2 Issues for the current backlog. Category:Quality Week Issues that should be considered for quality week labels Apr 27, 2020
@nkolev92 nkolev92 removed this from the 6.0 milestone Apr 27, 2020
@joserubenh
Copy link

Currently I'm using the following code when dynamically loading packages.

Dim source = New SourceRepositoryProvider(settings, Repository.Provider.GetCoreV3())
Dim repos = source.GetRepositories()

How can I get the default nuget.org repositories without using sourceRepositoryProvider and avoid the Obsolete Warning?.
Could you provide an example please?.

@joserubenh
Copy link

I've found a solution: Just use the respository Factory.
Dim repository = Repository.Factory.GetCoreV3("https://api.nuget.org/v3/index.json")
I hope this helps

@joergrosenkranz
Copy link

There does not seem to be a solution for the old implementation, that infers the sources from the NuGet.config of a directory and uses all of them. Strange that this is obsolete without a replacement.

@joergrosenkranz
Copy link

Here is a solution that works for me:

var settings = Settings.LoadDefaultSettings(directoryUnderNuGetConfig);

var repositories = new PackageSourceProvider(settings)
	.LoadPackageSources()
	.Where(source => source.IsEnabled)
	.Select(source => Repository.Factory.GetCoreV3(source.Source));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Category:Quality Week Issues that should be considered for quality week Functionality:SDK The NuGet client packages published to nuget.org Priority:2 Issues for the current backlog. Product:VS.Client Type:Bug
Projects
None yet
Development

No branches or pull requests

7 participants