Bug when Disposing ServiceClient

[tomasvokal]

Hello, there is a bug in disposing of service client.

We are using the service client to serve OBO (on-behalf) request proxy to Dataverse. A Call is requiring a Dataverse token from ITokenAcquisition from Microsoft.Identity.Web. After a call is done, we try to dispose the service client that is handled as transient reference. However, this produces ObjectDisposedException.

We are using service client constructor with tokenProviderFunction. In similar manner:

ServiceClient(dataverseUri, async (instanceURI) =>
{
    var parsedUri = new Uri(instanceURI);
    return await _tokenAcquisition.GetAccessTokenForUserAsync( new[] { $"{parsedUri.GetLeftPart(UriPartial.Authority)}//.default");
}, useUniqueInstance: true, logger: _logger);

There is a problem here in ServiceClient, where it tries to dispose property of _connectionSvc field. https://github.com/microsoft/PowerPlatform-DataverseServiceClient/blob/master/src/GeneralTools/DataverseClient/Client/ServiceClient.cs#L2336

This is very bad pattern since there is not knowing how the property implemented. In this case like this: https://github.com/microsoft/PowerPlatform-DataverseServiceClient/blob/master/src/GeneralTools/DataverseClient/Client/ConnectionService.cs#L415

Getting the property results to calling tokenProviderFunction again, but at the time it is called the request context is already disposed, thus there is no user to get token for.

I suggest removing lines for the disposing property of _connectionSvc and just let the _connectionSvc do its own cleanup since only that one is responsible of how it is implemented.

[MattB-msft]

@tomasvokal We are looking though the dispose logic currently.
It has existed for far longer than we offered the function injection feature.. It exists primarily to clean up the loggers which consume a lot of memory, and release any IO the client is using.

To be clear so we can get the right type of repro and test case built.. you are seeing a situation where:

1. you have a class you created that is exposing a GetToken Type function
2. you pass the method to the ServiceClient on Create
3. you dispose the ServiceClient ( via a using or directly )
4. the class hosting the GetToken Function and its host class is being disposed as well?

is that accurate?

thanks

[tomasvokal]

@MattB-msft we are using this as part of a WebApi application. But for sake of testing your assumption is right.

We are using scoped IoC for service client. In other words we have a provider always supplies a single version on ServiceClient for service a call and then it disposes the ServiceClient once the call is served.

We are getting the token from _tokenAcquisition From Microsoft Identity. Internally this uses IHttpContextAccessor.

Whole problem is that IHttpContextAccessor is disposed before we are disposing ServiceClient. However, disposing service client invokes function for getting token that invokes a resource holding disposed reference.

Problem is not so much a Dispose itself, but the fact that WebClientGetter is invoking a logic and that logic is outside the class scope. This will always lead to unpredictable results.

[MattB-msft]

Thanks Tomasvokal,

We took a look at this more deeply and setup a few repro's.

In your example above, you have a delegate that is returning the access token, where that delegate is being instanced in line with the call to the constructor. Thus, scope wise, its part of the ServiceClient object itself and subject to being deleted by the serviceclient when it cleans itself up.

If the scope of that Function pointer (delegate) is outside the scope of the serviceclient, then it should not be subject to being collected during the client's cleanup leg, Just the reference to the function is collected.

The use of Dispose though is targeted specifically for Unmanaged resources, In the most 'usual' case in the ServiceClient, that is for the Text Writers that are used as parts of loggers. We do that to prevent memory creep on long running processes and to hint the .net Garbage collector that the object and all of its assets can be collected and released.

In your example, Can you move that into a function that you have declared as part of your containing process? That will move it out of the scope of the ServiceClient.

For example:

Func<string, string> getToken = async (instanceURI) =>
{
    var parsedUri = new Uri(instanceURI);
    return await _tokenAcquisition.GetAccessTokenForUserAsync( new[] { $"{parsedUri.GetLeftPart(UriPartial.Authority)}//.default");
}

ServiceClient(dataverseUri, getToken , useUniqueInstance: true, logger: _logger);

Set up that way, it should not be affected by the dispose of the ServiceClient or its clones.

[MattB-msft]

Going to close this out now.. Please reopen it if you are still seeing the issue.