Home: Remove or improve configuration change events

[zivkan]

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]

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]

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.

[JakeSays]

@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]

@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.