Memory leak when an exception is raised during execution of SqlCommand with CancellationToken

[olegkv]

Description

When using CancellationToken, an exception raised by sqlCommand.ExecuteNonQueryAsync() causes memory leaks.
When not using CancellationToken, an exception raised by sqlCommand.ExecuteNonQueryAsync() does not cause any memory leaks.

More details about context

When we run a background process which executes sql commands in regular (scheduled) manner (execute-sleep-execute), and using global CancellationToken, process would go out of memory if some commands generate errors.

To reproduce

Run the code below. Comment and uncomment calls of ExecuteNonQueryAsync to see the difference between using CancellationToken and not using it.

CancellationToken stoppingToken = new CancellationTokenSource().Token;
using var connection = new SqlConnection("Data Source=localhost");
while (!stoppingToken.IsCancellationRequested)
{
	try
	{
		using var command = connection.CreateCommand();
		string myString = new string('*', 1000000);
		command.CommandText = "select * from Table1 where Name = @p1";
		var tvpParam = command.Parameters.AddWithValue("@p1", myString);
		tvpParam.SqlDbType = SqlDbType.VarChar;
		//memory leak
		await command.ExecuteNonQueryAsync(stoppingToken);
		//no memory leak
		//await command.ExecuteNonQueryAsync();
	}
	catch
	{
	}
}

Expected behavior

When using CancellationToken, memory usage should be the same as in the case of not using CancellationToken

Further technical details

Microsoft.Data.SqlClient version: (4.1.0)
.NET target: (Net 5 and 6)
Operating system: (Windows)

[ErikEJ]

There is no (4.8.3) version of Microsoft.Data.SqlClient. are you testing against the deprecated System.Data.SqlClient ?

[olegkv]

I've corrected SqlClient version. It is version 4.1.0 of Microsoft.Data.SqlClient.

[olegkv]

On first glance, it looks like you just need to add
a call to registration.Dispose();
to this catch block:
https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs#L2573

Because if BeginExecuteNonQueryAsync throws an exception, ContinueWith (which calls registration.Dispose()) will not be called.
FromAsync documentation:
"The beginMethod delegate is started on the thread that FromAsync is running on. This method throws any exceptions thrown by the beginMethod."
https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskfactory.fromasync?view=net-6.0#system-threading-tasks-taskfactory-fromasync(system-func((system-asynccallback-system-object-system-iasyncresult))-system-action((system-iasyncresult))-system-object)

[olegkv]

Same with InternalExecuteReaderAsync, seems like registration.Dispose() is missed in catch block:
https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs#L2675

[Kaur-Parminder]

@olegkv Thanks for the Repro, I can see the memory usage going quickly over a GB with tokens option whereas without token it remains under 100 MB even running for few minutes.

The behavior is same for System.Data.SqlClient and earlier version of Microsoft.Data.SqlClient 3.1.0 as well.

I will investigate further to find the cause.

[Wraith2]

On first glance, it looks like you just need to add a call to registration.Dispose(); to this catch block: https://github.com/dotnet/SqlClient/blob/main/src/Microsoft.Data.SqlClient/netcore/src/Microsoft/Data/SqlClient/SqlCommand.cs#L2573

Because if BeginExecuteNonQueryAsync throws an exception, ContinueWith (which calls registration.Dispose()) will not be called. FromAsync documentation: "The beginMethod delegate is started on the thread that FromAsync is running on. This method throws any exceptions thrown by the beginMethod." https://docs.microsoft.com/en-us/dotnet/api/system.threading.tasks.taskfactory.fromasync?view=net-6.0#system-threading-tasks-taskfactory-fromasync(system-func((system-asynccallback-system-object-system-iasyncresult))-system-action((system-iasyncresult))-system-object)

This looks like the probable fix but there is a complication that we must make sure we don't double dispose the registration variable. The registration itself does not report whether it has been disposed so we will need to carry and check that state along with the registration.

[Wraith2]

@olegkv can you verify if the same behaviour occurs with ExecuteReader? And would you be willing to try some dev builds if I open a PR to address this? There is a way to eliminate quite a few allocations in ExecuteNonQuery that I hadn't got around to doing yet because I'm waiting on source merges between netcore/netfx but it would be good to check if the rewrite I have planned fixes the problem.

[olegkv]

@Wraith2 Yes, same issue happens with ExecuteReaderAsync too. That matches with source code for InternalExecuteReaderAsync missing registration.Dispose() call.

Also, you said:
"This looks like the probable fix but there is a complication that we must make sure we don't double dispose the registration variable. The registration itself does not report whether it has been disposed so we will need to carry and check that state along with the registration."

But MS documentation explicitly says that components must allow multiple Dispose calls:
"If an object's Dispose method is called more than once, the object must ignore all calls after the first one. The object must not throw an exception if its Dispose method is called multiple times."

https://docs.microsoft.com/en-us/dotnet/api/system.idisposable.dispose?redirectedfrom=MSDN&view=net-6.0#remarks

[olegkv]

@Wraith2 Yes, sure I can try some dev builds. Anyone can try it easily, just copy the code from original post above and run it, comment and uncomment calls with token and without token and see the difference.