Throw NpgsqlException instead of InvalidOperationException on a broken connection#6599
Draft
irrationalmentality wants to merge 1 commit into
Draft
Conversation
…connection
When a connection was broken (e.g. by a keepalive failure), CheckReady/CheckOpen treated the Broken state identically to Closed and threw InvalidOperationException ("Connection is not open"). Connection errors should surface as NpgsqlException so callers can handle them as connection failures and recover.
Split the Broken case from Closed in both checks and throw NpgsqlException with a message pointing to the logs. The original break reason is logged when the connector breaks but is not reachable from the connection (which is detached on break), matching the maintainer guidance on npgsql#5605.
Also remove the stale doc note claiming ConnectorState.Broken is not implemented yet.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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 #5605.
Problem
When a connection is broken (e.g. by a keepalive failure),
CheckReady/CheckOpentreated theBrokenstate the same asClosedand threwInvalidOperationException("Connection is not open"). Connection errors are supposed to surface asNpgsqlException, so callers can catch them at a higher level, dispose their DB resources and reconnect —InvalidOperationExceptioncan't reasonably be handled that way.Change
Brokencase fromClosedin bothCheckReadyandCheckOpen.Brokennow throwsNpgsqlException;Closed(never opened / closed after normal use) keeps throwingInvalidOperationException("Connection is not open").ConnectorState.Broken("This state isn't implemented yet").Re: the two questions in my issue comment — the original exception can't be attached as
InnerExceptionat the connection level without extra plumbing (glad to add that as a follow-up if you'd like), and bothCheckReadyandCheckOpenare covered.Behavioral change
This changes the exception type for operations on a broken connection from
InvalidOperationExceptiontoNpgsqlException(NpgsqlException : DbException, socatch (DbException)now catches it whilecatch (InvalidOperationException)no longer does). Flagging in case it warrants thebreaking changelabel / a release-notes entry — it targets 11.0.Tests
Added
Broken_connection_throws_NpgsqlException_not_InvalidOperationExceptioninConnectionTests. Ran it plus the fullConnectionTestsand transaction test classes against a local PG 17: 224 passed, 0 failed (only the expected platform skips).🤖 Generated with Claude Code