You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
InvalidOperationException instead of NpgsqlException when client disconnects #5605
using System;
using System.Threading;
using Npgsql;
class Test2
{
static int Main(string[] args)
{
var connectionString = new NpgsqlConnectionStringBuilder {
Host = "test-server",
Database = "postgres",
Username = "postgres",
Password = "123456",
Pooling = false,
CommandTimeout = 0,
KeepAlive = 30,
TcpKeepAlive = true,
TcpKeepAliveTime = 30
};
var connection = new NpgsqlConnection();
connection.ConnectionString = connectionString.ConnectionString;
connection.Open();
try {
using (var c = new NpgsqlCommand("SELECT 'dummy'", connection)) {
c.ExecuteNonQuery();
}
Console.Write("disconnect the this client from the network and press enter when done so...");
// note: you can also work on localhost and stop the server instead of disconnecting the network
Console.ReadLine();
Console.WriteLine("waiting 1 minute.");
Thread.Sleep(60_000);
Console.WriteLine("BeginTransaction");
connection.BeginTransaction();
}
catch (NpgsqlException ex) {
Console.WriteLine("Expected Exception: " + Environment.NewLine + ex);
}
catch (Exception unexpected) {
Console.WriteLine("Unexpected Exception:" + Environment.NewLine + unexpected);
}
return 0;
}
}
The issue
According to the doc, exceptions related to connection errors should be wrapped in a NpgsqlException exception.
This makes perfectly sense as this will allow catching NpgsqlException at a relatively high level in the code,
dispose all DB related stuff and trying to create a new connection.
However in the example above, an InvalidOperationException will be thrown; this is definitely not an exception that should be handled and ignored at a higher level.
I think it is incorrect to throw a InvalidOperationException and it should be NpgsqlException with possibly containing the IOException. (If the connection would be used again after handling the NpgsqlException, InvalidOperationException would be correct.)
System.InvalidOperationException: Connection is not open
at Npgsql.ThrowHelper.ThrowInvalidOperationException(String message)
at Npgsql.NpgsqlConnection.CheckReady()
at Npgsql.NpgsqlConnection.BeginTransaction(Boolean async, IsolationLevel level, CancellationToken cancellationToken)
at Npgsql.NpgsqlConnection.BeginTransaction(IsolationLevel level)
at Npgsql.NpgsqlConnection.BeginTransaction()
at Test2.Main(String[] args) in Test2.cs:line 35
This sounds reasonable, I've marked it as an enhancement.
We won't have access to the reason the connection was broken (in this case due to keepalive) but it'll be good to throw the right type. The message should contain some pointers to check the logs etc.
Following up on @NinoFloris's note above: my plan is to stop treating a Broken
connection the same as a Closed one in the connection state checks
(CheckReady / CheckOpen), and throw an NpgsqlException instead of InvalidOperationException when the connection is in the Broken state.
Two things I'd like to confirm before opening a PR:
When the original exception that broke the connector is still available
(e.g. the IOException from a keepalive failure), should it be attached as
the InnerException?
Is there anything beyond CheckReady / CheckOpen you'd want covered here?
Activity
NinoFloris commentedon Mar 12, 2024
This sounds reasonable, I've marked it as an enhancement.
We won't have access to the reason the connection was broken (in this case due to keepalive) but it'll be good to throw the right type. The message should contain some pointers to check the logs etc.
iliyas86 commentedon Apr 18, 2025
Is there any plans to fix this issue?
irrationalmentality commentedon Jun 1, 2026
Hi! I'd like to take this one.
Following up on @NinoFloris's note above: my plan is to stop treating a
Brokenconnection the same as a
Closedone in the connection state checks(
CheckReady/CheckOpen), and throw anNpgsqlExceptioninstead ofInvalidOperationExceptionwhen the connection is in theBrokenstate.Two things I'd like to confirm before opening a PR:
(e.g. the
IOExceptionfrom a keepalive failure), should it be attached asthe
InnerException?CheckReady/CheckOpenyou'd want covered here?I'll follow up with a draft PR including a test.
Throw NpgsqlException instead of InvalidOperationException on broken …