StateChange event no longer fires when connection is observed to be killed

[madelson]

Steps to reproduce

Here is an NUnit test that reproduces the issue for me. As mentioned in the comments, it passes using 4.1.4 of Npgsql and fails when updated to 5.0.1.1.

        [Test]
        public async Task TestExecutingQueryOnKilledConnectionFiresStateChanged()
        {
            using var stateChangedEvent = new ManualResetEventSlim(initialState: false);

            using var connection = new NpgsqlConnection("valid connection string here");
            await connection.OpenAsync();
            connection.StateChange += (o, e) => stateChangedEvent.Set();

            using var getPidCommand = connection.CreateCommand();
            getPidCommand.CommandText = "SELECT pg_backend_pid()";
            var pid = (int)(await getPidCommand.ExecuteScalarAsync());

            Assert.AreEqual(ConnectionState.Open, connection.State);

            // kill the connection from the back end
            using var killingConnection = new NpgsqlConnection(TestingPostgresDb.ConnectionString);
            await killingConnection.OpenAsync();
            using var killCommand = killingConnection.CreateCommand();
            killCommand.CommandText = $"SELECT pg_terminate_backend({pid})";
            await killCommand.ExecuteNonQueryAsync();

            Assert.ThrowsAsync<PostgresException>(() => getPidCommand.ExecuteScalarAsync());
            Assert.AreNotEqual(ConnectionState.Open, connection.State);

            Assert.IsTrue(stateChangedEvent.Wait(TimeSpan.FromSeconds(5))); // assertion passes in 4.1.4, fails in 5.0.1.1
        }

The issue

The connection's StateChange event should fire whenever the connection state is observed to have changed, such as when running a query against a connection that turns out to have been killed. This worked in the version of Npgsql I was upgrading from and works with MSFT's SqlServer ADO providers.

Further technical details

Npgsql version: 4.1.4 -> 5.0.1.1
PostgreSQL version: 12
Operating system: Windows 10

[madelson]

@vonzshik any chance that this will make it into 5.0.2?

[vonzshik]

This is a very tricky issue. While this event does fire on 4.*, it's only so because when the connection is broken we immediately close it. It seems, @roji did change this behavior (most likely because of the multiplexing), but in my opinion it does make more sense unlike the former.
I think, to fix this we have to add ConnectionState.Broken for the connection (that is, set it when a bound connector is broken) and after that fire the event.
But first things first, I would like to hear what @roji thinks.

[YohDeadfall]

Not tricky, am fixing it already.

[vonzshik]

Well, if you go the way I described, you have to fix every place where we check the connection's state, add at the very least 2 new events (open->broken and broken->closed, maybe even broken->open, or just throw an exception to close the connection before opening). And I might have missed something, so no that simple 😄
Also, @roji might have different ideas...

[vonzshik]

Also, multiplexing - the connector might break while it's not bound to the connection, so we cannot just call the event from the Break method.

[madelson]

@vonzshik this likely isn't helpful, but the behavior of the SqlServer providers seems to be that the StateChange event only gets called as part of a user interacting with the connection. So it will be called if we try to execute a command and it fails because the connection is now broken, but it doesn't promise to fire the instant that the connection actually breaks. Possibly implementing to this spec would reduce the complexity somewhat?

[YohDeadfall]

Specs:

The StateChange event occurs when the state of the connection changes from closed to opened or from opened to closed.

And for the State property only 3 values are available: Open, Connecting or Close. If connector's state doesn't fall into Open category, then it's closed. Therefore, changing the full state to Broken means that the connection is closed.

[roji]

Yeah, this was indeed the result of the multiplexing work, we even had a test previously checking the event firing, and I removed that check. I admit I don't remember the full reasoning around everything any more.

I can't really see a reason not to have this event fire when the connector is broken, and @YohDeadfall's implementation in #3451 looks good to me... I've also double-checked just in case, and SqlClient indeed emits a Closed event if a command is executed on a connection that had been broken before...

@vonzshik do you have any specific concerns, do you want to give #3451 a review?