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

Fix memory leak root cause: not disposing subscriptions to TP invalidation events #477

Closed
wants to merge 3 commits into from

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented May 31, 2015

This fixes #106 - unsubscribes the language service from the "invalidate" signal coming from the type providers when a project is done with.

The failure to unsubscribe causes a very small memory leak in a well-functioning type provider which disposes of its resources explicitly

It can cause a very large memory leak when a type provider is badly written and which expects the GC finalization mechanisms to dispose of its resources. This is because the subscription causes a live handle to be kept to otherwise-collectable resources that would trigger invalidation events (like file-system watchers). If the type provider has explicitly cleaned up file system watchers, then it's not too bad. But if it hasn't and expects them to be collected, then they are kept alive, often along with all the resources of the type provider as well.

I've added testing and checked it compiles and tests appear to be passing on my machine.

This bug is severe in the sense it's a pretty easy and common form of memory leak in type provider implementations, and has historically been a contributing factor to very major leaks as documents in #106 and the linked bugs.

@dsyme dsyme changed the title Fix memory leak: not disposing even subscriptions to invalidation events Fix memory leak root cause: not disposing subscriptions to TP invalidation events May 31, 2015
@dsyme
Copy link
Contributor Author

dsyme commented May 31, 2015

tests now appear to be passing

@dsyme
Copy link
Contributor Author

dsyme commented May 31, 2015

Spoke to soon, the added test is not passing

@dsyme
Copy link
Contributor Author

dsyme commented May 31, 2015

OK, should pass now.

@ovatsus
Copy link

ovatsus commented May 31, 2015

Any guidance on how to properly clean up file system watchers?

@dsyme
Copy link
Contributor Author

dsyme commented May 31, 2015

@ovatsus - I think it's as you expect: when the type provider instance is disposed, you dispose the file system watchers and keep no live global handles to them. I believe FSharp.Data does this correctly. AFAIU the problems above only happen if the file system watchers are not disposed explicitly but rather you are relying on GC to cleanup them up.

@latkin
Copy link
Contributor

latkin commented Jun 1, 2015

Code changes make sense. I want to try this out in real VS and test out the repro described in the bug.

@latkin
Copy link
Contributor

latkin commented Jun 2, 2015

This doesn't seem to hurt, but it also doesn't appear to make much of a dent in the overall trend of VS memory consumption. VS still grows to 1-2GB working set/private bytes after 5-10 reloads/rebuilds of the VFPT solution (as described in the issue).

@dsyme
Copy link
Contributor Author

dsyme commented Jun 2, 2015

Correct, as far as I'm aware the type providers used by that solution have already been fixed to do explicit cleanup of resources. The problem with TPs was real, but by no means the only cause of memory use.

@latkin
Copy link
Contributor

latkin commented Jun 2, 2015

Got it, LGTM.

@MattGertz
Copy link

Approved 6/2 by ML Shiproom.

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

Successfully merging this pull request may close these issues.

None yet

5 participants