Memory Leak when using ContextPropagationInterceptor

[aputlock]

What version of gRPC and what language are you using?

C#
gRPC codegen compiled with Grpc.Tools 2.40.0
Affected process importing Grpc.AspNetCore 2.50.0

What operating system (Linux, Windows,...) and version?

Windows

What runtime / compiler are you using (e.g. .NET Core SDK version dotnet --info)

.net7

What did you do?

My service implements a server streaming call, which as part of its implementation polls an rpc from another service. We also have ContextPropagationInterceptor added to the pipeline.

Simplified example:

public override async Task Stream(StreamRequest request, IServerStreamWriter<StreamResponse> responseStream, ServerCallContext context)
{
    using CancellationTokenSource cts = new CancellationTokenSource();
    while (!context.CancellationToken.IsCancellationRequested)
    {
        // _otherService is a codegen'd grpc client
        var response = await _otherService.GetStateAsync(new GetStateRequest(), new Metadata(), cancellationToken: cts.Token);
        await responseStream.WriteAsync(response, context.CancellationToken);
    }
}

What did you expect to see?

All resources cleaned up

What did you see instead?

Process memory increases over time -- memory dump + ETW trace indicates there's a leak of Linked2CancellationTokenSource objects (+ associated CallbackRegistration, CallbackNode).

ContextPropagationInterceptor allocates a LinkedCancellationTokenSource and sets it up to be disposed when the containing GrpcCall is disposed. However, it doesn't seem like the call is ever implicitly disposed.

Using unary calls as an example:

grpc-dotnet/src/Grpc.AspNetCore.Server.ClientFactory/ContextPropagationInterceptor.cs

Line 153 in c9c902c

linkedCts = CancellationTokenSource.CreateLinkedTokenSource(serverCallContext.CancellationToken, options.CancellationToken);

grpc-dotnet/src/Grpc.AspNetCore.Server.ClientFactory/ContextPropagationInterceptor.cs

Lines 115 to 121 in c9c902c

	return new AsyncUnaryCall<TResponse>(
	responseAsync: call.ResponseAsync,
	responseHeadersAsync: UnaryCallbacks<TResponse>.GetResponseHeadersAsync,
	getStatusFunc: UnaryCallbacks<TResponse>.GetStatus,
	getTrailersFunc: UnaryCallbacks<TResponse>.GetTrailers,
	disposeAction: UnaryCallbacks<TResponse>.Dispose,
	CreateContextState(call, cts));

grpc-dotnet/src/Grpc.AspNetCore.Server.ClientFactory/ContextPropagationInterceptor.cs

Line 236 in c9c902c

internal static readonly Action<object> Dispose = state => ((ContextState<AsyncUnaryCall<TResponse>>)state).Dispose();

grpc-dotnet/src/Grpc.AspNetCore.Server.ClientFactory/ContextPropagationInterceptor.cs

Lines 211 to 215 in c9c902c

	public void Dispose()
	{
	Call.Dispose();
	CancellationTokenSource.Dispose();
	}

I can work around this by adjusting my code, but it doesn't feel expected/ergonomic from a user's perspective:

public override async Task Stream(StreamRequest request, IServerStreamWriter<StreamResponse> responseStream, ServerCallContext context)
{
    using CancellationTokenSource cts = new CancellationTokenSource();
    while (!context.CancellationToken.IsCancellationRequested)
    {
        using var unaryCall = _otherService.GetStateAsync(new GetStateRequest(), new Metadata(), cancellationToken: cts.Token);
        var response = await unaryCall;
        await responseStream.WriteAsync(response, context.CancellationToken);
    }
}

Particularly considering the comment on AsyncUnaryCall:Dispose:

grpc-dotnet/src/Grpc.Core.Api/AsyncUnaryCall.cs

Lines 138 to 147 in c9c902c

	/// <summary>
	/// Provides means to cleanup after the call.
	/// If the call has already finished normally (request stream has been completed and call result has been received), doesn't do anything.
	/// Otherwise, requests cancellation of the call which should terminate all pending async operations associated with the call.
	/// As a result, all resources being used by the call should be released eventually.
	/// </summary>
	/// <remarks>
	/// Normally, there is no need for you to dispose the call unless you want to utilize the
	/// "Cancel" semantics of invoking <c>Dispose</c>.
	/// </remarks>

It seems like either AsyncUnaryCall should be automatically disposing its AsyncCallState at some point, or ContextPropagationInterceptor has a faulty assumption that AsyncCallState will be disposed in normal control flow.

[JamesNK]

Thanks for the investigation. Fix here: #2421