Retry not working with SqlDataAdapter.Fill(table) (.net framework)

[MichaelAzzer]

When use retry with command and execute command with DataReader
like

 using (SqlDataReader reader = command.ExecuteReader())
                    {
                        table.Load(reader);
                        
                    }

it's working
but if I used SqlDataAdapter to fill table it's not working

[JRahnama]

@MichaelAzzer we will look at it and will get back to you.

[lcheunglci]

Hi, is there a sample project that I can use to reproduce this?

[MichaelAzzer]

Hi, is there a sample project that I can use to reproduce this?

Yes, I prepared a simple sample to produce the problem

this zipped file contains WinForms sample with database script.sql
sample.zip

[lcheunglci]

Thanks @MichaelAzzer , I was able to reproduce it locally with your sample project. I'll take a look and debug it.

[MichaelAzzer]

Thanks @MichaelAzzer , I was able to reproduce it locally with your sample project. I'll take a look and debug it.
It's nice to hear that, thank you very much for your time

[lcheunglci]

Hi @MichaelAzzer , I noticed that the exception being thrown is a timeout exception:

Microsoft.Data.SqlClient.SqlException: 'Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.'

I change the command.CommandTimeout = 1; to command.CommandTimeout = 5; and it worked. I also tried the "working" version with SqlDataReader and it also threw the same timeout exception until I changed the command timeout.

Can you try changing the command timeout to a longer duration and try again and let me know if it works?

[MichaelAzzer]

Hi @MichaelAzzer , I noticed that the exception being thrown is a timeout exception:

Microsoft.Data.SqlClient.SqlException: 'Execution Timeout Expired.  The timeout period elapsed prior to completion of the operation or the server is not responding.'

I change the command.CommandTimeout = 1; to command.CommandTimeout = 5; and it worked. I also tried the "working" version with SqlDataReader and it also threw the same timeout exception until I changed the command timeout.

Can you try changing the command timeout to a longer duration and try again and let me know if it works?

Yes, you're right
I decreased command timeout intentionally to test retry but when timeout elapsed exception occurs and retry didn't work also command_Retry event didn't trigger

private static void command_Retry(object sender, SqlRetryingEventArgs e)
        {
            SqlException ex = (SqlException)e.Exceptions[0];
            MessageBox.Show(ex.Message);
        }

[lcheunglci]

Thanks for the clarification. I understand the issue now i.e. it's not triggering the retry callback from the SqlCommand.RetryLogicProvider from SqlDataAdapter where as SqlDataReader does trigger the retry callback. I see that SqlDataAdapter inherits from DbDataAdapter which is where the Fill logic is implemented from the System.Data.Common namespace which I don't see source code for. The DataTable where it calls Load belongs to System.Data namespace which the implementation details is also not available, so my guess is that the DataTable.Load(IDataReader) utilizes the SqlCommand.RetryLogicProvider where as DbDataAdapter.Fill(DataTable) does not, which I don't think we can fix on the SqlClient side. @JRahnama correct me if I'm making the wrong assumption.

[lcheunglci]

After reading the implementation of SqlCommand.ExecuteReader(), I see that there's logic that checks if a RetryLogicProvider is provided in the SqlCommand and if so, the SqlReader returned is wrapped with RetryLogicProvider; however, in the core3.1 implementation of DbDataAdapter.FillInternal where the SqlDataAdapter inherits from, it grabs an IDataReader from ICommand.ExecuteReader. I think the issue is usage of the new keyword in the SqlCommand.ExecuteReader() which means the ICommand used in the DbDataAdapter won't be able to use RetryLogicProvider instead it uses the one from DbCommand, which doesn't use the RetryLogicProvider. I see that @DavoudEshtehari was the last person making a change to that line. I'll double check with him to see why it was implemented this way, and if we can override the ExecuteReader so it doesn't use the DbCommand's version of it.

[MichaelAzzer]

After reading the implementation of SqlCommand.ExecuteReader(), I see that there's logic that checks if a RetryLogicProvider is provided in the SqlCommand and if so, the SqlReader returned is wrapped with RetryLogicProvider; however, in the core3.1 implementation of DbDataAdapter.FillInternal where the SqlDataAdapter inherits from, it grabs an IDataReader from ICommand.ExecuteReader. I think the issue is usage of the new keyword in the SqlCommand.ExecuteReader() which means the ICommand used in the DbDataAdapter won't be able to use RetryLogicProvider instead it uses the one from DbCommand, which doesn't use the RetryLogicProvider. I see that @DavoudEshtehari was the last person making a change to that line. I'll double check with him to see why it was implemented this way, and if we can override the ExecuteReader so it doesn't use the DbCommand's version of it.

I appreciate this hard work, and I want to thank 😊 you very much

[lcheunglci]

You're welcome. After checking in with @DavoudEshtehari , he mentioned that the implementation was there since the repo was created, and the reason why we had to new was because ExecuteReader in DbCommand was not virtual and therefore we cannot override it. Unfortunately, SqlDataAdapter won't be able to support the RetryLogicProvider, and perhaps, it should be added to the documentation, so that others are aware of this limitation.

[roji]

the reason why we had to new was because ExecuteReader in DbCommand was not virtual and therefore we cannot override it.

Note that ExecuteReader in DbCommand (and implements the IDbCommand.ExecuteReader discussed above) simply calls ExecuteDbDataReader, which is itself abstract. In other words, IDbCommand.ExecuteReader should indeed return an instance of SqlDataReader.

(Providers are expected to indeed replace ExecuteReader with new, but only to return the provider-specific implementation of DbDataReader (e.g. SqlCommand.ExecuteReader returns SqlDataReader, exposing SqlClient-specific APIs). This is a general pattern in ADO.NET for implementing a sort of "covariance".)

Note also that DbDataAdapter in general is a layer on top of DbCommand and DbDataReader, so at least in principle whatever works in a DbCommand implementation should also work via DbDataAdapter. There may be some additional complexity in SqlClient specifically that makes this difficult though - I'm not familiar with the internals.

[MichaelAzzer]

Sorry ,I have two more questions
1-Is this behavior in .net framework only or in both .net framework and .net core?
2-Is this problem will be fixed in the future or still one of the Microsoft.Data.SqlClient limitations?

[lcheunglci]

For your first question, since this behaviour was identified in your sample project, which is a .NET Framework 4.8 WinForms desktop application and DbDataAdapter from .NET Framework and DbDataAdapter from .NET Core 3.1 are nearly the same, I think the behaviour is consistent for both .NET Core and .NET Framework. As for you second question, here's my assumption and it does not reflect the actual decision on whether it'll be fixed or kept as it is. I think might be possible to fix it if we make SqlCommand inherit directly from ICommand instead of DbCommand to enable the RetryLogicProvider; however, there's a high probability that SqlCommand depends on logic from DbCommand and it may introduce some side effects that we may not expect, which is hopefully why we have unit tests, so I feel like the fix is risky, so the safer option is to say it's a limitation in Microsoft.Data.SqlClient and update the documentation, but any of the maintainers can chime in to this discussion.

[JRahnama]

@David-Engel Can you please check this and guide us to the next step?

[lcheunglci]

We found that the DataAdapter has Fill with an overload that takes in a SqlDataReader/IDataReader; however, it is not public i.e. protected virtual and DbDataAdapter does not make it public. I did a test and verified that by using the base version of Fill with the SqlDataReader works with the following example by adding this in SqlDataAdapter

public int Fill(DataTable dataTable, SqlDataReader reader)
{
	return base.Fill(dataTable, reader);
}

and updating your sample code to this

using (SqlDataAdapter adapter = new SqlDataAdapter(command))
{
	adapter.Fill(table, command.ExecuteReader());
}

and I got it to trigger the MessageBox from your callback method connection_Retry

Hence, there are 2 approaches to add RetryLogicProvider support to SqlDataAdapter.Fill in MDS.

1. Use the example above and make a Fill(DataTabe, SqlDataReader) method in SqlDataAdapter
2. Use the new keyword to hide/"override" the 5 public Fill methods from DbDataAdapter

In that case, I'll remove my PR to update the docs. @JRahnama @David-Engel what do you think?

[MichaelAzzer]

@lcheunglci It's very nice 👌 to hear that I will try this solution
Finally I want to thank 😊 you for your hard work.

[JRahnama]

hiding is not recommended. Net6 will give us CA1061 if we hide the method and it is suggested not to suppress this rule. Although we have couple of them inside the driver in SqlTransaction.

[roji]

I haven't been following this in detail, but FWIW hiding base methods - so as to return the more specific provider type - is a pretty standard part of how the ADO.NET API is designed: it allows for a form of covariance.

For example, DbCommand has a protected abstract ExecuteDbDataReader method, as well as a public non-virtual ExecuteReader which calls the former. Both methods are typed to return DbDataReader.

Now, an ADO.NET provider must override ExecuteDbDataReader with its implementation, which will return a provider-specific implementation of DbDataReader (e.g. SqlDataReader). But the public user AIP - ExecuteReader - still returns DbDataReader, meaning that users can't access any provider-specific APIs. So the provider hides ExecuteReader with its own version, which returns the specific type (SqlDataReader), allowing users to automatically get access to provider-specific features without downcasting etc.

(again, I have no idea how any of this is related to retries - just commenting on the method hiding part!)