Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Work in Progress] SqlException State 35 #619

Closed
wants to merge 1 commit into from

Conversation

alishahahmed
Copy link

@alishahahmed alishahahmed commented Nov 9, 2022

Description

Added retry logic on Sql connection OpenAsync method when the method throws a SqlException with State 35 to take care of the following exception:

"A connection was successfully established with the server, but then an error occurred during the login process. (provider: TCP Provider, error: 35 - An internal exception was caught)"

Related issues

AB#96340

Semver Change

Skip

…throws a SqlException with State 35 to take care of the following exception:

"A connection was successfully established with the server, but then an error occurred during the login process. (provider: TCP Provider, error: 35 - An internal exception was caught)"
@alishahahmed alishahahmed requested review from jovinson-ms and a team November 9, 2022 20:54
/// Retries have been added to cater to the following exception:
/// "A connection was successfully established with the server, but then an error occurred during the login process.
/// (provider: TCP Provider, error: 35 - An internal exception was caught)"
/// </remarks>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to add a test to exercise this logic specifically?

This would retry on any internal exception - is this the only one that exists or is there potentially others with state 35?
Are there exceptions aside from those with state 35 that should be retried?

{
int attempts = 1;

await Policy.Handle<SqlException>(se => se.State == 35)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no need to await this. Simply return the Task

int attempts = 1;

await Policy.Handle<SqlException>(se => se.State == 35)
.WaitAndRetryAsync(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: indent by 1 here and below

_logger.LogWarning(
exception,
"Attempt {Attempt} of {MaxAttempts} to open Sql connection.",
attempts++,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't the retryCount parameter provide this?

/// <remarks>
/// Retries have been added to cater to the following exception:
/// "A connection was successfully established with the server, but then an error occurred during the login process.
/// (provider: TCP Provider, error: 35 - An internal exception was caught)"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure this is something we want to retry? The SQL Client library already retries a number of different errors? Do we know why this isn't retried? Is this a "retryable" exception? For example, I see an old issue in the issues page for this problem when moving from .NET 5 -> 6: dotnet/SqlClient#1402. Unsure if it's related, but I am reluctant to add new retries if we aren't quite sure why they're happening.

@alishahahmed
Copy link
Author

Closing as this will be taken care of by Daniel's work: https://microsofthealth.visualstudio.com/Health/_workitems/edit/96670.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants