Marshal.GetFunctionPointerForDelegate leaks memory

[jeffschwMSFT]

Copied from a VSFeedback item.

A customer noticed that repeated calls to a p/invoke that took a delegate appeared to be leaking memory. The underlying cause is the new way that we allocate umentrythunks and how GetFunctionPointerForDelegate caches them. Previously, the same thunk was returned and used when run in a tight loop (see code below). Now a new thunk is used every time, which results in memory seemingly increasing.

using System.Runtime.InteropServices;
 
namespace test
{
   class Program
   {
      static void Main(string[] args)
      {
         while (true)
         {
            var delg = new EnumWindowsProc(MyEnumProc);
            IntPtr ptr = Marshal.GetFunctionPointerForDelegate( delg ); 
            GC.Collect(); 
            GC.WaitForPendingFinalizers(); 
            GC.Collect(); 
         }
      }
      private static int MyEnumProc(IntPtr hwnd, IntPtr lParam)
      {
         return 0;
      }

      private delegate int EnumWindowsProc(IntPtr hwnd, IntPtr lParam);
   }
}

[jeffschwMSFT]

Introduced by f9a161c

[jkotas]

Now a new thunk is used every time, which results in memory seemingly increasing.

I see the thunks getting reused after a while. Yes, we use some extra fixed amount of memory to get better diagnostics after f9a161c, but it is not a memory leak.

I agree that we have a memory leak, but the problem are leaking GC handles. There is nothing that calls ~UMEntryThunk destructor. This bug was introduced in Windows Phone CoreCLR port and it was in .NET Core since day 1.

[mateoatr]

Yes, looks like the thunks are good. We create up to 64 and then reuse them, as @jkotas noted. However, everytime Marshal.GetFunctionPointerForDelegate gets called, we create a new weak long handle which seems a bit odd to me; since we're reusing the thunks, shouldn't we reuse the handles too?

My commit stops the leak, although I'm not sure if it is the correct approach.

[jkotas]

@mateoatr Thanks for looking into this.

My commit stops the leak, although I'm not sure if it is the correct approach.

We want to destroy the GC handle for thunks that are not in use. Otherwise, the GC handles would be keeping user objects alive for no good reason that would be a different type of leak.

I think that the fix should be to move the code from ~UMEntryThunk to UMEntryThunk::Terminate, and delete ~UMEntryThunk.

[Gladskih]

What version of .net core has fix for this?

[jkotas]

This fix will ship in .NET 5. It was not back-ported to older versions.

[avl]

Is this bug in .NET Framework 4.6.2? I'm about to deploy a long-running program that does a lot of GetFunctionPointerForDelegate, should I expect problems?

[jkotas]

This bug was in .NET Core only.

[cnx-jd]

Is there a work around for this at all? Or any chance of getting this patched in .Net Core 3.X? We encountered this in a new project we're working on with .Net Core 3.1.
Having to wait for .Net 5.0 (or more likely .Net 6.0 for LTS) would probably mean we have to revert to .Net Framework for our release, something we'd prefer not to do.

[AaronRobinsonMSFT]

@cnx-jd Thanks for asking about this again. Given the recurrent requests and the trivial change, we have decided to consider this for .NET Core 3.x servicing. The discussion for servicing and decision can be followed at dotnet/coreclr#28074.