Cancel running command when a (pooled) connection is closed

[roji]

Spawned from #1800. When closing a pooled connection, check if a command is currently running and cancel if so.

Keep in mind that cancellation is a very expensive operation (opening a new TCP socket to the server) and should only happen if necessary.

[YohDeadfall]

If it's expensive would it be better to cancel the running command when getting a connection?

[roji]

@YohDeadfall it's going to cost the same whether it happens on close or on open... In other words, it's merely a question of which thread is going to pay the price - the one closing or the one opening.

If we do this, I think it's quite important to do this on close rather than on open - after a connection is returned to the pool, it may spend quite a bit of time there before it's reallocated again. We don't want the command to keep running while the connection is idle in the pool (it may be eating resources at the server etc...).

But there's another way to look at this - when closing a pooled connection with a running command, Npgsql could asynchronously perform the cancellation. In other words, it would release the closing thread immediately and execute the cancellation without blocking anyone. This would remove any serious perf hit - connections would simply take a bit longer to become available again as idle in the pool, but opening and closing threads would not be (directly) affected.

If we go down this route, it seems important to make sure cancellation completed fully before actually marking the connection as idle and putting it into the pool. We don't want any non-idle connections to get in there.

Implementation-wise, this logic should probably happen as part of NpgsqlConnection.Close(), the pool itself should be kept unaware of all of this.

[mdalepiane]

I have some questions about this:

1. Should we call pg_cancel_backend(pid int) to cancel the running command?
   a. If so can the PID be found anywhere or a query is needed to get it?
   b. If not what should be used to cancel the running command?
2. Can FullState == (ConnectionState.Open | ConnectionState.Executing) be used to identify when there is a running command?

[roji]

@mdalepiane the standard way to cancel a command in PG is described here - this isn't with pg_cancel_backend, which requires an open/idle connection. Npgsql already implements this for when you explicitly call NpgsqlCommand.Cancel, or trigger an asynchronous cancellation token - just not when a connection is returned to the pool.

@vonzshik you may be interested in this given all the cancellation work you've been doing recently.

[vonzshik]

A few points:
a) NpgsqlConnection.Close already ensures that there is no running query when the connection is returned to the pool
b) if we do go the way of cancelling a running query on NpgsqlConnection.Close, it will be a very big breaking change (for example, sql client doesn't cancel the query).

[roji]

@vonzshik you're right about the breaking change, it's a good question. I did feel at the time that closing a connection implies cancelling a running command - and I still think that makes sense to some extent. But I agree that it's probably not worth the breaking change to people.

However, one thought in this area is that the waiting for the query to complete can be done asynchronously and "out of band". In other words, Close can be made 100% synchronous (for pooled connections), without waiting for any sort of cleanup, but rather firing it off in parallel. Once the cleanup is complete and the resultset is consumed, the connection is returned to the pool - but there's no reason for use code to wait on that (in Close). Of course, it could be claimed that this is also a small breaking change, in case user code expects that when Close completes the query has finished. That doesn't sound like a very strong argument to me.

But I'm not sure this is a very valuable optimization - and users can implement it themselves as well if that's what they want. What do you think @vonzshik?

[vonzshik]

I did feel at the time that closing a connection implies cancelling a running command - and I still think that makes sense to some extent. But I agree that it's probably not worth the breaking change to people.

I do agree, it kind of makes sense. But that's not something I'm comfortable to break without a good reason (and right now we don't have one).

However, one thought in this area is that the waiting for the query to complete can be done asynchronously and "out of band". In other words, Close can be made 100% synchronous (for pooled connections), without waiting for any sort of cleanup, but rather firing it off in parallel. Once the cleanup is complete and the resultset is consumed, the connection is returned to the pool - but there's no reason for use code to wait on that (in Close). Of course, it could be claimed that this is also a small breaking change, in case user code expects that when Close completes the query has finished. That doesn't sound like a very strong argument to me.

There is still one problem - the NpgsqlDataReader.Dispose, as it consumes the resultset. Without changing this, it leaves us with only one possible use case:

using var reader = cmd.ExecuteReader();
// some code, which doesn't read the response completely
connection.Close();

And I'm not really sure it's really worth adding a cancellation on NpgsqlConnection.Close just for this.

[roji]

Yeah, I agree there's not much reason to do anything here. I'll close this, but if anyone wants to provide another perspective please post here and we'll reconsider.