Memory Leak in the services (GoogleAdsServiceClientBase)

[cbeauchemin]

Describe the bug:

We are actually using the Google Ads library for a great number of client. Each of those clients have their own set of credentials to use. Our app is not responsible of refreshing those token (it can but its not what we usually do). Another application is responsible for creating the initial refresh token and can refresh the access token when asked to. Basically, when our app need a credential, we ask it from a central application and it give us back the access token we need. The way the GoogleAdsClient is done is more oriented toward using it for one set of credential and for one client. Proof of that is that the class GoogleAdsConfig contain a field named RefreshToken. The class GoogleAdsClient is built using a GoogleAdsConfig. Of course, we have the possibility to new up some GoogleAdsClient each time we need one and that is the way we actually use it. So new GoogleAdsClient and new GoogleAdsConfig each time we need one. The problem we faced in the past is that Google was caching the channels it create for each of the services under a GoogleAdsClient. As thing progressed, when the library v7 was released, we could set the configuration property UseChannelCache to false to prevent services to stack up in the global static caching variable for them. We don’t want to cache things because credential could change and our app is really not responsible for that.

We made our own GoogleAdsConfig derived from the original one as described in this link https://developers.google.com/google-ads/api/docs/client-libs/dotnet/performance.

Using the technique described, we were having the control upon where the credential was coming from when it was requested and in the same time, re-use the same access token among all our systems for the same client instead of emitting a new access token for each service each time we need one.

The actual problem we are facing is that even when setting ChannelCaching to false, there seem to be a leak from somewhere in the GoogleAdsLibrary. The UserCredential emitted by the GoogleAdsConfig get trapped in the GoogleAuthInterceptors. Those interceptor are kept alive by each service channel.

Steps to Reproduce:
Use a derived GoogleAdsConfig that redefine the CreateCredentials

`

     public NewGoogleAdsConfig(IConfigurationSection configurationSection, int clientIdentifier): base(configurationSection)
     {
            _clientIdentifier = clientIdentifier;
            UseChannelCache = false;
     }


    protected internal ICredential CreateCredentialsInternal()
    {
        var flow = new GoogleAuthorizationCodeFlow(
            new GoogleAuthorizationCodeFlow.Initializer
            {
                ClientSecrets = new ClientSecrets
                {
                    ClientId = OAuth2ClientId,
                    ClientSecret = OAuth2ClientSecret
                },
                Scopes = new[] {OAuth2Scope},
                HttpClientFactory = new HttpClientFactory()
            });

       var refreshToken = <Some code to get some refresh token>

        OAuth2RefreshToken = refreshToken ;
        if (credentials.LoginCustomerId == null)
        {
            throw new InvalidOperationException(
                $"Provider account {_providerAccountId} do not have a LoginCustomerId which is required for new Google Ads Api.");
        }
        LoginCustomerId = credentials.LoginCustomerId.Value.ToString();
        OAuth2Mode = OAuth2Flow.APPLICATION;

        return new UserCredential(
                  flow,
                  DEFAULT_USER_ID,
                  new TokenResponse()
                  {
                      RefreshToken = OAuth2RefreshToken,
                  });
    }

`

instanciate a lot of GoogleAdsClient with new GoogleAdsConfig each time.
Do this like 100 times
You will see that even if the GoogleAdsClient and GoogleAdsConfig get garbage collected, the Credential stay there

Solution Proposed

I managed to fix it by shutdowning the channel inside of the services generated by the client.

I copied all the source code to make my test so I am sure of the result. Here is my suggestions:

GoogleAdsServiceClientBase:

Keep track of the Channel created by the GoogleAdsServiceClientFactory and implement the IDisposable that will call Channel.ShutdownAsync().Wait(); when disposed

GoogleAdsServiceClientFactory:

When the function GetService is called, set the GoogleAdsServiceClientBase property to keep track of the channel. Also, the line : .Intercept(GoogleAdsGrpcInterceptor.GetInstance(config));
Create a static interceptor for the first GoogleAdsConfig created. I should be new each time because configuration instance is not always the same. So I would do instead: .Intercept(new GoogleAdsGrpcInterceptor (config));

Expected behavior:
Grips to the UserCredential get release by the channel/interceptor so it can be garbage collected in the same time as the GoogleAdsClient

Client library version and API version:
Client library version: 10.2.0
Google Ads API version: 9
.NET version: 5.0
Operating system (Linux, Windows, ...) and version (if the bug is platform-specific): Windows

Request/Response Logs:
Not related to that part

[cbeauchemin]

[AnashOommen]

@cbeauchemin I'll investigate this further, but my understanding is that Channels are expensive to create, so it is recommended that we leave them alone and do not shut them down when a service goes out of scope. @jskeet @jtattermusch can you please confirm?

[jskeet]

Assuming this is Grpc.Core, my understanding is that unless there are unique channel options coming into play (which may be part of the additional code in the Ads client), new .NET Channel objects end up piggy-backing onto the same underlying network connection, which makes them somewhat less expensive than they would otherwise be.

But even so, in general I would recommend keeping a Channel alive for as long as possible, rather than deliberately shutting them down and recreating them.

[cbeauchemin]

Ok thanks for answering quickly. As explained in the first description, i create a GoogleAdsClient and GoogleAdsConfig each time i process something for a client. For that client, i use 2 or 3 services. Those services asks the GoogleAdsConfig to Create them a credential object. Credential objects are never left alone and never garbage collected. The solution to close the Channel was the only solution i found to make it happen. Otherwise, memory is crashing the service.

Option to not cache channel should not create a leak. Actually, with my modified version of your library, i have no problem anymore. It should be the decision of the developper to close the channel or not. Maybe the Dispose pattern i not the right way. Just a function inside the servicebase could work for me. I am open to solutions/avenues but keeping a Credential object that is out of date is not an option for me.

(100000 clients * (size of the interceptors in memory + size of the credential object in memory + size of the channel in memory))

[AnashOommen]

Oh... I didn't realize that it caused your server to eventually crash... I'll take another look at the code to see how best to integrate your suggestion.

[jtattermusch]

By GoogleAuthInterceptors, do you mean one of https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Auth/GoogleAuthInterceptors.cs?

Also, it would be good to know how the credentials get applied to the underlying grpc Channel. Are the ICredential credentials created in the method CreateCredentialsInternal (as mentioned in your code snipped) being wrapped as CallCredentials or as ChannelCredentials at the grpc level? (see e.g. here: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Auth/GoogleGrpcCredentials.cs)

[cbeauchemin]

Yes, its really GoogleAuthInterceptors. I did not created classes with that name.

The credential i created is derived from Google.Apis.Auth.OAuth2.ICredential.

If you can, provide me a safe way to send you my code so you can have a better idea.

Thanks

Claude

[jtattermusch]

Yes, its really GoogleAuthInterceptors. I did not created classes with that name.

The credential i created is derived from Google.Apis.Auth.OAuth2.ICredential.

If you can, provide me a safe way to send you my code so you can have a better idea.

Thanks

Claude

Feel free to send an e-mail to the e-mail address associated with my github account (can be found e.g. int the git commit log of https://github.com/grpc/grpc/commits/master).

Otherwise, just knowing how the Google.Apis.Auth.OAuth2.ICredential credential you created is passed to the client library would probably be enough.

[cbeauchemin]

Hi Jan,
It seems to me that your github email is private. You can write it here or contact me at claude.beauchemin@newfold.com.
Thanks

[cbeauchemin]

Hi Jan,
I sent you an email with the part of the code implicated.
Thanks
Claude

[jtattermusch]

@cbeauchemin based on the code you provided, it seems that this is pretty much the place where gRPC Channel instances are created:

google-ads-dotnet/src/Lib/CachedChannelFactory.cs

Line 139 in b1c7566

return new Channel(uri.Host, uri.Port, channelCredentials, options);

The way the credential objects are applied to the grpc channels is here:

google-ads-dotnet/src/Lib/CachedChannelFactory.cs

Line 121 in b1c7566

GoogleGrpcCredentials.ToChannelCredentials(config.Credentials);

(they are converted to gRPC ChannelCredentials).

The ChannelCredentials objects will stay alive for the entire duration of the Channel they've been assigned to. Unless the channels are being shutdown (channel.ShutdownAsync()) at some point, that would definitely explain why they the credential objects and the wrapped credentials are being "leaked". That behavior is not surprising and to me it sounds like "working as intended".

To react to the question from @AnashOommen ( #355 (comment))
Yes, new channels should be created with care because there's some performance/memory overhead in creating a new channel, but creating multiple channels or shutting down existing channel and creating new ones from time to time certainly isn't illegal (I suspect doing so isn't strictly speaking necessary to achieve the behavior the user needs in this particular issue, but that's a different story).

The channels that get created should eventually be cleaned up (by calling channel.ShutdownAsync()), otherwise the resources associated with the channel will not get released (and it looks like a leak). So never calling ShutdownAsync() is basically a bug.

[AnashOommen]

@jtattermusch I'll fix this in that case. One question though -- does channel.ShutdownAsync() exist in grpc-dotnet? If not, this fix can be pointless in the future... @jskeet is cross-library channel.ShutdownAsync() something you'd expose in Gax.Grpc?

[jskeet]

Yes, ChannelBase.ShutdownAsync exists: https://github.com/grpc/grpc/blob/master/src/csharp/Grpc.Core.Api/ChannelBase.cs#L65

[jtattermusch]

@AnashOommen in grpc-dotnet the GrpcChannel instances should be Disposed once you stop using them.
https://github.com/grpc/grpc-dotnet/blob/f238855fcb77cec01e87eb9cc08680c3420c132f/src/Grpc.Net.Client/GrpcChannel.cs#L594
(there's no need for the async shutdown mechanism).

Note that GrpcChannel from grpc-dotnet implements ChannelBase, but currently when you invoke channelBase.ShutdownAsync() on a grpc-dotnet channel, that will basically fall back to the default implementation here (noop): https://github.com/grpc/grpc/blob/d303f81ab4775da2eb1f40055f716ded24a4e7f7/src/csharp/Grpc.Core.Api/ChannelBase.cs#L72
Since this behavior is probably counterintuitive, we might wanna consider changing the ShutdownAsync() behavior on GrpcChannel (override the default noop impl).
@JamesNK WDYT?

[AnashOommen]

@cbeauchemin I'll incorporate your fix in the next library release (along with v10_1 API release).

[cbeauchemin]

@AnashOommen Great news!

[AnashOommen]

This is fixed in 12.0.0.