SqlDataReader.DisposeAsync throw SqlException Execution: Timeout Expired. The timeout period elapsed prior to completion of the operation or the server is not responding

[LIFEfreedom]

Describe the bug

I got an exception when trying to call DisposeAsync on a SqlDataReader

Unhandled exception. Microsoft.Data.SqlClient.SqlException (0x80131904): Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.
 ---> System.ComponentModel.Win32Exception (258): Unknown error 258
   at Microsoft.Data.SqlClient.SqlInternalConnection.OnError(SqlException exception, Boolean breakConnection, Action`1 wrapCloseInAction)
   at Microsoft.Data.SqlClient.TdsParser.ThrowExceptionAndWarning(TdsParserStateObject stateObj, Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParserStateObject.ThrowExceptionAndWarning(Boolean callerHasConnectionLock, Boolean asyncClose)
   at Microsoft.Data.SqlClient.TdsParserStateObject.ReadSniError(TdsParserStateObject stateObj, UInt32 error)
   at Microsoft.Data.SqlClient.TdsParserStateObject.ReadSniSyncOverAsync()
   at Microsoft.Data.SqlClient.TdsParserStateObject.TryReadNetworkPacket()
   at Microsoft.Data.SqlClient.TdsParserStateObject.TryPrepareBuffer()
   at Microsoft.Data.SqlClient.TdsParserStateObject.TryReadByteArray(Span`1 buff, Int32 len, Int32& totalRead)
   at Microsoft.Data.SqlClient.TdsParser.TrySkipValue(SqlMetaDataPriv md, Int32 columnOrdinal, TdsParserStateObject stateObj)
   at Microsoft.Data.SqlClient.TdsParser.TrySkipRow(_SqlMetaDataSet columns, Int32 startCol, TdsParserStateObject stateObj)
   at Microsoft.Data.SqlClient.TdsParser.TryRun(RunBehavior runBehavior, SqlCommand cmdHandler, SqlDataReader dataStream, BulkCopySimpleResultSet bulkCopyHandler, TdsParserStateObject stateObj, Boolean& dataReady)
   at Microsoft.Data.SqlClient.SqlDataReader.TryCloseInternal(Boolean closeReader)
   at Microsoft.Data.SqlClient.SqlDataReader.Close()
   at Microsoft.Data.SqlClient.SqlDataReader.Dispose(Boolean disposing)
   at System.Data.Common.DbDataReader.DisposeAsync()
   at Lib.Sql.SqlListener.SubscribeAsync(Int32 attempts)
   at Lib.Sql.SqlListener.SqlDependencyOnChange(Object sender, SqlNotificationEventArgs args)
   at System.Threading.Tasks.Task.<>c.<ThrowAsync>b__139_1(Object state)
   at System.Threading.ThreadPoolWorkQueue.Dispatch()
ClientConnectionId:3a8049f8-7536-482c-828c-86b31b7e0b38
Error Number:-2,State:0,Class:11

To reproduce

Start Sql Server, start command execution when Sql Server is under heavy load.

My fuction:

private async Task SubscribeAsync(int attempts = 0)
{
    SqlConnection connection = null;
    SqlCommand command = null;
    SqlDataReader reader = null;
    try
    {
        connection = new SqlConnection(Options.ConnectionString);
        command = new SqlCommand(Options.Query, connection) { CommandTimeout = Options.CommandTimeout };
        await connection.OpenAsync().ConfigureAwait(false);
        var sqlDependency = new SqlDependency(command);
        sqlDependency.OnChange += SqlDependencyOnChange;
        CurrentSqlDependency = sqlDependency;
        reader = await command.ExecuteReaderAsync().ConfigureAwait(false);
    }
    catch (Exception ex)
    {
        if (attempts++ >= Options.MaxAttempts)
        {
            StopListen();
        }
        else
        {
            await Task.Delay(Options.RetryInterval).ConfigureAwait(false);
            await SubscribeAsync(attempts).ConfigureAwait(false);
        }
    }
    finally
    {
        if (reader is object)
            await reader.DisposeAsync().ConfigureAwait(false);
        if (command is object)
            await command.DisposeAsync().ConfigureAwait(false);
        if (connection is object)
            await connection.DisposeAsync().ConfigureAwait(false);
    }
}

Expected behavior

I expected the application to continue working, but not an exception being thrown.

Further technical details

Microsoft.Data.SqlClient version: 2.0.0 (Assembly Microsoft.Data.SqlClient, Version=2.0.20168.4)
.NET target: Сore 3.1.6
SQL Server version: SQL Server 2016
Operating system: Ubuntu 18.04

[JRahnama]

@LIFEfreedom Can you provide more information on your connection string options and the query or the issue happens with any kind of query?

[LIFEfreedom]

@JRahnama
Connection string:
Data Source=SQLServer;Initial Catalog=DbName;User ID=USER;Password=MYPASS;TrustServerCertificate=Yes
Query:
SELECT Id FROM dbo.Events
This has happened for the first time and has not happened again, since the peak load on the database does not happen often.

[LIFEfreedom]

@JRahnama are there any news

[DenSmoke]

Same problem. Last release didn't fix the issue.

[JRahnama]

@LIFEfreedom on some other issues with similar error message we noticed that wrapping your connection, command and readers in using blocks will reduces the problem significantly. By doing that we ensure that all resources are disposed when the related part is disposed. Can you try that please?

[DenSmoke]

@JRahnama This solution didn't help. Main problem is that Dispose and DisposeAsync shouldn't throw exceptions. https://docs.microsoft.com/ru-ru/previous-versions/visualstudio/visual-studio-2015/code-quality/ca1065-do-not-raise-exceptions-in-unexpected-locations?view=vs-2015&redirectedfrom=MSDN#dispose-methods

[JRahnama]

@JRahnama Javad Rahnama (Simba Technologies Inc) Vendor This solution didn't help. Main problem is that Dispose and DisposeAsync shouldn't throw exceptions. https://docs.microsoft.com/ru-ru/previous-versions/visualstudio/visual-studio-2015/code-quality/ca1065-do-not-raise-exceptions-in-unexpected-locations?view=vs-2015&redirectedfrom=MSDN#dispose-methods

I agree. As per my understanding .NET5 runtime is enforcing that behavior. We will adjust the code based on this rules to be compatible with .NET5 runtimes in future when we add .NET5 support to our driver. I do not have a exact date for that change, but I will look into changing this particular one to see if we can get the exception out of the dispose method in a simple PR at the middle of next week.

[JRahnama]

@DenSmoke, @LIFEfreedom can you try with the fix proposed in PR #901 as well to see if that solves this issue please? That PR solves some issue with leaked exceptions in the driver.

[David-Engel]

Looking at the history of SqlDataReader.cs, it looks like the Dispose() method was added in the initial commit of the NetCore code in 2015 but was never part of the NetFx code. I suspect it might have been added to ensure cleanup of server-side resources but the Close() call should have been wrapped in a try/catch to ensure Dispose() never throws. I don't know why it would have been added in NetCore but not NetFx. Makes me wonder whether the correct fix is to remove the method completely or add the try/catch around Close(). I lean towards adding the try/catch as there is a lot of important cleanup that happens in Close().

[ajcvickers]

@David-Engel If I remember correctly, then DbDataReader is always disposable in .NET Framework because it inherits from Component, which is disposable. Initially Component was not included in .NET Core, and hence at that time DbDataReader was made explicitly disposable.

There is a general problem here in that since all Db... classes were inherited from Component, it means that they were all disposable. This means some things are not designed to be disposable at the ADO.NET level, and yet have always implemented the IDisposable interface anyway. So, for Core, we tried to look at the existing provider ecosystem and decide which objects really need to be disposed. DbDataReader is in that list since some providers associate native resources with the data reader. So even though some providers such as SqlClient traditionally don't need to dispose the DbDataReader, at the ADO.NET level the contract from the consuming side is that it must be disposed.

/cc @bricelam @roji

[roji]

That's odd... I'm pretty sure that even back in .NET Framework with System.Data.SqlClient, disposing a reader on a non-MARS SqlConnection would allow executing another command on that same connection; not doing so would trigger a "reader is already open" exception.

As @ajcvickers says, some ADO.NET objects are indeed IDisposable even though disposing them generally isn't required (this is usually the case of DbCommand); one could that this is a good thing given that System.Data is an abstraction, and some provider out there may need their DbCommand to be disposed too for some reason. But in any case, I think DbDataReader generally must be disposed across database providers before another command can be executed on the connection.

And as is standard in .NET (long before .NET 5), Dispose should indeed never throw.

[JRahnama]

@roji, @David-Engel while going through this issue, I noticed there are lots of IDisposable-type fields in the internal classes of SqlDataReader such as AAsyncCallContext. According to Microsoft Documentations this needs to be done inside the Dispose method of the very same class which is not done in our case. The other example could be at line 4254 of SqlDataReader and this happens in lots of other places which an IDisposable object is created, but never disposed. Should we dispose them either by wrapping them in using block or just dispose them. Am I missing something here?
The other question I had, inside the very same class there is method ClearCore(), this copies the IDisposable field to another field and disposes the copy and set the original to null! Is there a pattern or logic behind it that I cannot see or this is a mistake?

[Wraith2]

I noticed there are lots of IDisposable-type fields in the internal classes of SqlDataReader such as AAsyncCallContext

Those are part of async calls so the disposable objects outlive the stack frame they are instantiated in. In particular the cancellation registration is created and becomes part of the call context and is then disposed of when the call has completed in CompleteAsyncCall which calls Dispose, which calls Clear.

The other question I had, inside the very same class there is method ClearCore(), this copies the IDisposable field to another field and disposes the copy and set the original to null! Is there a pattern or logic behind it that I cannot see or this is a mistake?

The copy is taken so we can access the reference, the field is nulled so we have a safely cleared object for re-use (which is the major reason for the class existing), then dispose is called. At this point if dispose throws an error we can't end up with an invalid state on the call context. If dispose takes a long time it doesn't matter because the context was already ready to re-use. You could argue that dispose should never throw but the existence of this thread shows that it might be worth assuming that it could. So the logic there is thought through and deliberate and you need to be aware of the way that async calls are constructed and flow to follow the lifetimes.