Fix LoginWithFailover missing parser state check#4140
Open
paulmedynski wants to merge 1 commit intomainfrom
Open
Fix LoginWithFailover missing parser state check#4140paulmedynski wants to merge 1 commit intomainfrom
paulmedynski wants to merge 1 commit intomainfrom
Conversation
LoginWithFailover() was missing the _parser?.State check that LoginNoFailover() already has. This caused transient errors (40613, 42108, 42109) to trigger failover alternation instead of being thrown immediately for the outer ConnectRetryCount loop to handle. Added the same parser state check so that login-phase errors (parser open) throw immediately while network errors (parser closed) continue failover alternation. Fixes #4139
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a failover login behavior discrepancy in SqlConnectionInternal.LoginWithFailover() by aligning its error-handling logic with LoginNoFailover(), ensuring login-phase transient/server errors are thrown for the outer ConnectRetryCount retry loop instead of triggering failover alternation.
Changes:
- Added a
_parser?.State is not TdsParserState.Closedcheck inLoginWithFailover()’sSqlExceptioncatch block. - Documented the rationale in-code to distinguish login-phase errors from network-level failures during failover alternation.
Comment on lines
+3665
to
+3669
| // If state != closed, indicates that the parser encountered an error while | ||
| // processing the login response (e.g. an explicit error token). Transient | ||
| // network errors that impact connectivity will result in parser state being | ||
| // closed. Only network-level errors should trigger failover alternation; | ||
| // login-phase errors (like transient errors) should be thrown so they can |
There was a problem hiding this comment.
Minor grammar/clarity: the new comment reads "If state != closed, indicates...". Consider rephrasing to "If state != Closed, it indicates..." (and optionally reference TdsParserState.Closed explicitly) to match the code and improve readability.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #4139
Description
LoginWithFailover()inSqlConnectionInternal.cswas missing the_parser?.Statecheck thatLoginNoFailover()already has. This caused transient errors (40613, 42108, 42109) to trigger failover alternation instead of being thrown immediately for the outerConnectRetryCountloop to handle.The code comment in
LoginWithFailover()even notes: "The logic in this method is paralleled by the logic in LoginNoFailover. Changes to either one should be examined to see if they need to be reflected in the other."Changes
Added the same
_parser?.State is not TdsParserState.Closedcheck toLoginWithFailover()catch block, consistent withLoginNoFailover():Testing
ConnectionFailoverTestspass on net8.0 and net9.0TransientFault_WithUserProvidedPartner_RetryDisabled_ShouldFailnow correctly fails with the expectedSqlException