Use weak references to IncrementalBuilder in invalidation callbacks

[dsyme]

This is defensive coding to reduce the severity of any leaks or coding mistakes in type providers or the F# compiler.

Coding mistakes can be

1. If TP instances have mistakenly kept themselves alive by F# compiler not disposing them correctly
2. If a TP registers itself in global state, e.g. in a callback in a file watch timer or stores itself in global state in its own DLL
3. if a TP saves one of its generated ProvidedTypeDefinition in a global cache in its own DLL

In these cases, the TP holds an invalidation callback object in its Invalidate event. This invalidation callback should not keep compiler state such as IncrementalBuilder alive if it is otherwise eligible for GC.

An appropriate defensive coding strategy is to add a weak reference at this point.

This doesn't solve an underlying problem, but is a reasonable step for reducing the impact of coding mistakes in type providers.

[dsyme]

Hmmm, one error:

Failed   TypeProvider.Disposal.SmokeTest1
Error Message:
 System.Reflection.TargetInvocationException : Exception has been thrown by the target of an invocation.
  ----> System.Exception : expected configuration object to be disposed

We've seen this test fail before, and it seems unrelated to the PR, which only deals with invalidation, but perhaps there is a connection. Will re-run.

[KevinRansom]

I expect this needs localizing.

[dsyme]

It's not user visible

[cartermp]

@dsyme @TIHan can we get a writeup of the problem, with the dump analysis as data, about the memory leak and its impact (preferably in a separate issue). I'd like to track what all is going in and what it fixes in #6265

[dsyme]

@dsyme @TIHan can we get a writeup of the problem, with the dump analysis as data, about the memory leak and its impact (preferably in a separate issue).

I updated the issue description to make the purpose of this PR clearer

[cartermp]

Link #6409 so we have a concrete issue and milestone to associate it with

[KevinRansom]

@dsyme, Do we know how this can happen If TP instances have mistakenly kept themselves alive by F# compiler not disposing them correctly ? and do we have an issue to fix it?

[dsyme]

@dsyme, Do we know how this can happen If TP instances have mistakenly kept themselves alive by F# compiler not disposing them correctly ? and do we have an issue to fix it?

1. We believe we have seen TP being kept alive and in traces for FSharp.Data, but we don't have a specific cause.

2. Looking at the FSharp.Data code it seems possible that the lifetime of the TP is being extended by it storing it into its own DLL global state, e.g. global static caches. However those weren't appearing as roots in the traces.

3. We've reviewed the F# compiler TP disposal code closely, e.g. @TIHan has been looking at it w.r.t. cancellation. We don't yet know of any specific problems but we do get occasional failures in the "Type Provider Smoke Tests" (which are intensive but not exhaustive of all possible code paths). Our intuition is that there must be a leak - and it is quite likely the F# compiler is responsible.

Basically we're looking through a lot the large-memory-use traces and noting various things. This is just one obvious defensive step that cropped up along the way when looking at one trace where uncollected IncrementalBuilder seemed to be rooted in the event handler callback. We never want that to be the sole root, and should always have done this defensive coding.

The tracking issue is #6409, it's the best we have for now since we don't know of any specific problems, except what we saw in one trace.

Have added #6410 as an issue for this PR.

[TIHan]

The defensive steps make sense to me. Though, it is still worrying that the traces indicate a leak that we don't know the cause.

It's incredibly difficult to pinpoint exactly why the leak is happening. It's obvious that Dispose is not being called on TcImports; calling Dispose gets rid of the strong reference that the type providers hold. So, we check to see who is responsible for calling Dispose on TcImports, and that is the IncrementalBuilder which is also responsible for creating a TcImports.

After carefully looking at the creation and disposal logic, I haven't been able to spot anything. At first, I thought it was the cancellation logic that is involved when building the TcImports in the IncrementalBuilder, but that seems fine and shouldn't be the cause of the leak. But, I have yet to carefully look at how the reference counting works in the IncrementalBuilder.

[cartermp]

@dsyme #6409 was already filed for this PR, shall we close that or the one you created?

[dsyme]

@dsyme #6409 was already filed for this PR, shall we close that or the one you created?

My bad, thanks

[TIHan]

I managed to get an assertion from IncrementalBuilder after opening and closing several different projects, including VisualFSharp:

This indicates we have a bug in reference counting somewhere.

[TIHan]

OK, I'm fine with this change as having a weak reference here makes sense.