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

Possible memory leak when used with ASP.NET Core DI #148

Open
simonhaines opened this issue Jul 4, 2022 · 2 comments
Open

Possible memory leak when used with ASP.NET Core DI #148

simonhaines opened this issue Jul 4, 2022 · 2 comments

Comments

@simonhaines
Copy link

While investigating an issue with high CPU usage, a memory dump was taken of a long-running ASP.NET Core 6 server application (see below) that uses the PubNubPCL library.

The heap has a lot of retained instances of objects related to the PubNub API, particularly PNConfguration which, after about 3 days of continuous operation, occupies 107MB of heap space.

I suspect the issue lies with the lifetime of the PubNub client in the DI container. We have a thin wrapper around the PubNub API that we inject with a Transient lifetime. The constructor creates a new PubNub object and configures it every time it is injected.

Based on the large number of PubnubApi.ConcurrentDictionary objects also on the heap, I believe the PubNub client is supposed to have a Singleton lifetime, but I could not find any guidance in the documentation regarding DI containers.

Can you confirm the expected lifetime of the PubNub client? Is it expected to be a Singleton, or can it be constructed as needed in a long-running service?

Here is the relevant part of the heap analysis created with dotnet-dump. The command used was dumpheap -stat -live (statistical analysis of live objects only).

              MT    Count    TotalSize Class Name
[snip]
00007f832b80f308   235225      7527200 PubnubApi.ConcurrentDictionary`2[[System.String, System.Private.CoreLib],[System.Net.HttpWebRequest, System.Net.Requests]]
00007f83294d3a10   235225      7527200 PubnubApi.NewtonsoftJsonDotNet
00007f832b827460        1      7786800 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[PubnubApi.ConcurrentDictionary`2[[System.String, System.Private.CoreLib],[System.Net.HttpWebRequest, System.Net.Requests]], PubnubPCL]][]
00007f832b826520        1      7786800 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[PubnubApi.EndPoint.TokenManager, PubnubPCL]][]
00007f832b8262a0        1      7786800 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[PubnubApi.IPubnubLog, PubnubPCL]][]
00007f832b826020        1      7786800 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[PubnubApi.PNConfiguration, PubnubPCL]][]
00007f83294d45b8   235225     13172600 PubnubApi.EndPoint.TokenManager
00007f8325678080     4495     13808408 System.Int32[]
00007f832b80b6a0   470451     15054432 PubnubApi.ConcurrentDictionary`2[[System.String, System.Private.CoreLib],[System.Boolean, System.Private.CoreLib]]
00007f832b8276e0        2     15573600 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[PubnubApi.ConcurrentDictionary`2[[System.String, System.Private.CoreLib],[System.Boolean, System.Private.CoreLib]], PubnubPCL]][]
00007f8325e89150   708563     17005512 System.Threading.Timer
00007f8326aada58   708775     17010600 System.Threading.TimerHolder
00007f83255c5290   708988     17015712 System.Object
00007f832b826f38   235225     18818000 System.Collections.Generic.Dictionary`2[[System.String, System.Private.CoreLib],[System.Net.HttpWebRequest, System.Net.Requests]]
00007f8325ba2890       17     33503088 System.Collections.Generic.Dictionary`2+Entry[[System.String, System.Private.CoreLib],[System.String, System.Private.CoreLib]][]
00007f83294d4898   708502     34008096 PubnubApi.EndPoint.TelemetryManager
00007f832a8c9368   470451     37636080 System.Collections.Generic.Dictionary`2[[System.String, System.Private.CoreLib],[System.Boolean, System.Private.CoreLib]]
00007f8325abac58    38012     39857914 System.Byte[]
00007f83266827b8   708749     45359936 System.Threading.TimerCallback
00007f8326682678   708860     68050560 System.Threading.TimerQueueTimer
00007f832567d2e0   740815     68704692 System.String
00007f83294d2820   708502    107692304 PubnubApi.PNConfiguration
Total 7963348 objects
@tvanengeland
Copy link

We recently ran into a similar issue like you have, though we did notice it a bit later on. Since PubNub's documentation still makes no reference about the lifetime or their Pubnub instances. This lead us to the following memory dump which we analyzed with perfview and resulted in the following image.
image

To reduce the amount of code changes that we had to make (Mostly in the creation of the Pubnub class instances) we decided to make a cleanup method which we have now implemented in every place a Pubnub class is created and made sure to call it right before the reference to that instance was lost. A bit like a dispose method would do on this class. Since the ConcurrentDictionary instances are private static fields on the PubnubCoreBase and TokenManager we decided to use reflection to just access the backing fields and clear those of the specified instance.

using System;
using System.Net;
using System.Reflection;
using PubnubApi;
using PubnubApi.EndPoint;

public static class PubnubHelper
{
    private static readonly FieldInfo PubnubInfoBackingField;
    private static readonly FieldInfo ChannelRequestBackingField;
    private static readonly FieldInfo ChannelGroupInternetStatusBackingField;
    private static readonly FieldInfo ChannelInternetStatusBackingField;
    private static readonly FieldInfo PubnubTokenMgrBackingField;
    private static readonly FieldInfo CurrentUserIdBackingField;
    private static readonly FieldInfo PubnubLogBackingField;
    private static readonly FieldInfo DTokenBackingField;

    static PubnubHelper()
    {
        // Derive PubnubCoreBase backingfields which hold on to data indefinitely 
        Type pubnubCoreBaseType = typeof(PubnubCoreBase);
        PubnubInfoBackingField = pubnubCoreBaseType.GetField("<pubnubConfig>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        ChannelRequestBackingField = pubnubCoreBaseType.GetField("<ChannelRequest>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        ChannelGroupInternetStatusBackingField = pubnubCoreBaseType.GetField("<ChannelGroupInternetStatus>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        ChannelInternetStatusBackingField = pubnubCoreBaseType.GetField("<ChannelInternetStatus>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        PubnubTokenMgrBackingField = pubnubCoreBaseType.GetField("<PubnubTokenMgrCollection>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        CurrentUserIdBackingField = pubnubCoreBaseType.GetField("<CurrentUserId>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
        PubnubLogBackingField = pubnubCoreBaseType.GetField("<pubnubLog>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);

        // Derive TokenManager backingfield which holds on to data indefinitely 
        Type tokenManagerType = typeof(TokenManager);
        DTokenBackingField = tokenManagerType.GetField("<dToken>k__BackingField", BindingFlags.NonPublic | BindingFlags.Static);
    }

    /// <summary>
    /// Operations for PubNub will persist various objects when the Pubnub instance is being used like in GrantToken or other operations that extend PubnubCoreBase.
    /// These objects in the PubnubCoreBase should be cleaned up when the pubnub instance is no longer necessary and the reference will be lost and that is what this method does.
    /// </summary>
    /// <param name="pubnub">A Pubnub instance that is used for doing operations with pubnub.</param>
    public static void CleanupPubnubInstanceRemnants(Pubnub pubnub)
    {
        if (pubnub is null)
        {
            return;
        }

        // PubnubCoreBase static fields
        var staticPubnubInfo = (ConcurrentDictionary<string, PNConfiguration>)PubnubInfoBackingField.GetValue(null);
        staticPubnubInfo?.TryRemove(pubnub.InstanceId, out _);

        var channelRequest = (ConcurrentDictionary<string, ConcurrentDictionary<string, HttpWebRequest>>)ChannelRequestBackingField.GetValue(null);
        channelRequest?.TryRemove(pubnub.InstanceId, out _);

        var channelGroupInternetStatus = (ConcurrentDictionary<string, ConcurrentDictionary<string, bool>>)ChannelGroupInternetStatusBackingField.GetValue(null);
        channelGroupInternetStatus?.TryRemove(pubnub.InstanceId, out _);

        var channelInternetStatus = (ConcurrentDictionary<string, ConcurrentDictionary<string, bool>>)ChannelInternetStatusBackingField.GetValue(null);
        channelInternetStatus?.TryRemove(pubnub.InstanceId, out _);

        var pubnubTokenMgr = (ConcurrentDictionary<string, TokenManager>)PubnubTokenMgrBackingField.GetValue(null);
        pubnubTokenMgr?.TryRemove(pubnub.InstanceId, out _);

        var currentUserId = (ConcurrentDictionary<string, UserId>)CurrentUserIdBackingField.GetValue(null);
        currentUserId?.TryRemove(pubnub.InstanceId, out _);

        var pubnubLog = (ConcurrentDictionary<string, IPubnubLog>)PubnubLogBackingField.GetValue(null);
        pubnubLog?.TryRemove(pubnub.InstanceId, out _);

        // TokenManager static fields
        var dtoken = (ConcurrentDictionary<string, string>)DTokenBackingField.GetValue(null);
        dtoken?.TryRemove(pubnub.InstanceId, out _);
    }
}

Hopefully PubNub will address this issue properly and either advise on using Pubnub instances in a DI container, add a fix for this issue to their Destroy method or make the class an IDisposable so it conforms better to C# standards. However for those who are also running into this issue, we fixed it in this way.

@simonhaines
Copy link
Author

Our issues were resolved by adding Pubnub to the services collection with singleton lifetime.

Until specified otherwise from Pubnub, the correct approach is:
IServiceCollection.AddSingleton(new Pubnub(new PNConfiguration(...))

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

No branches or pull requests

2 participants