Remove Configurable Retry Logic safety switch

[DavoudEshtehari]

The App Context switch "Switch.Microsoft.Data.SqlClient.EnableRetryLogic" will no longer be required to use the configurable retry logic feature. The feature is now supported in production. The default behavior of the feature will continue to be a non-retry policy, which will need to be overridden by client applications to enable retries.

Related links:

• Released: Microsoft.Data.SqlClient 3.0 Preview 1
• Introducing Configurable Retry Logic in Microsoft.Data.SqlClient v3.0.0-Preview1
• Configurable retry logic in SqlClient

[Wraith2]

Will this change the observed behaviour for existing users that do not have the feature enabled? Will they see commands retrying by default where they did not with previous releases?
Will this have a performance impact for users who do not have the feature enabled today?

[DavoudEshtehari]

My attempt was to take care of customers who don't use this feature. It shouldn't have a perf issue, though.
I'll run a perf test to find it out. In the meanwhile, I'd appreciate it if you could run your benchmark too.

[Wraith2]

If it's going to be the default then users will just have to live with it in terms of performance. My main concern is for code that doesn't expect to have retries enabled can easily be broken by the library changing to enable them by default.

[DavoudEshtehari]

It'll go into the retry path if any provider (internal/external) except a none-retriable which could be created by CreateNoneRetryProvider() function has been assigned to a connection or command.
We welcome any better idea.

[Wraith2]

If the feature needs to be more mainstream then adding a way to enable it which is easier to discover like a static property (or config file setting if no code changes is a required aspect) and then publicizing that would be preferable. It concerns me that a major behavioural change is going to be made without any evidence of user demand for it.

@roji will this change to enable retries by default have any impact on EFCore?

[roji]

It concerns me that a major behavioural change is going to be made without any evidence of user demand for it.

I tend to agree - especially since I think this feature provides an illusion of resiliency without being able to deliver on it (e.g. transactions, any errors happening after the application already read some rows...). Seems like this did become a GA quite quickly...

@roji will this change to enable retries by default have any impact on EFCore?

I don't think so, aside from any perf penalties this feature brings (I have no idea about that). As far as I understand, this feature doesn't change SqlClient's visible behavior in any way except for not surfacing certain exceptions when a retry succeeds under the hood... EF Core's own resiliency would continue to work on top of this - if the user opted into using it - so we'd have two different retry strategies at the same time at different levels of the stack.

@ajcvickers or @AndriySvyryd may have some other thoughts.

[ajcvickers]

It can potentially drastically change the amount of time for something to fail. For example, imagine that previously EF would retry 3 times, taking a total of 10 seconds before failing. (Numbers made up; I don't remember what the actual defaults are.) Now if SqlClient does several retries itself taking 10 seconds before failing and letting EF retry, now the total time to failure becomes 30 seconds--3 times longer than before. We had people run into this when the original connection retries were introduced. People reported applications hanging because things were being retried so many times.

I agree with @roji's other points, but this at the very least needs to be documented and publicized as a breaking change, since based on previous experience that's how many customers will experience it.

[David-Engel]

@Wraith2 @roji @ajcvickers

To clarify the behavior, we are not adding retries by default. This is simply removing the extra safety switch we put in front of the retry provider logic to ensure the "preview" feature didn't accidentally affect behavior in the GA release. We feel the feature is ready for GA, and there will still be zero retries by default, so no behavior change. There should be negligible impact on performance, if any. If users want to enable retries, they can do so via the app.config file. They just no longer need the extra AppContext safety switch.

[ErikEJ]

So it is "Enable Configurable Retry Logic to be enabled by default" 🤔

[David-Engel]

So it is "Enable Configurable Retry Logic to be enabled by default" 🤔

Maybe a better title for this PR would be "Remove Configurable Retry Logic safety switch".

[ajcvickers]

Thanks for clarifying that, @David-Engel

[roji]

Thanks @David-Engel, seems like I totally misunderstood.

[Wraith2]

Me too, though I was trying to get the answer about whether it was being enabled for every user by default. If it's disabled by default it's fine.