postgres: forcibly terminate connections when dropped

[benesch]

Hey @bikeshedder! Thanks for the awesome library—we've been using deadpool-postgres to great effect in @MaterializeInc for a while now. But we recently got bit by the issue described below, where dropping a deadpool_postgres::ClientWrapper does not actually force the connection to terminate.

We think this is enough of a footgun that it's worth changing the default behavior to forcibly terminate the connection, which is what this PR does. We'd be happy to rework this to make this a configurable option of the pool/client, if preferable—@umanwizard and I just couldn't come up with a good use case for when you'd want to opt out of this behavior, so we went with the simplest possible solution to start. What do you think?

---

This commit makes it so that dropping a deadpool_postgres::ClientWrapper forcibly terminates the PostgreSQL connection, even if there is outstanding work on the connection (e.g., a long running query).

Previously, the background connection task would continue to run until the outstanding work completed. This could cause the memory/TCP socket associated with the connection to effectively leak until that outstanding work completed. In the case of a runaway query (imagine, for example, SELECT pg_sleep(100000000)), that leak could potentially last forever.

[bikeshedder]

That's very similar to the issue that the redis crate (and all the other implementations) have. If a connection is used and the future is not pulled to completion there are two possible outcomes:

1. "crash and burn" - That's how it is implemented in redis-rs right now. If a client is dropped while a query is being processed the next user of that client receives the wrong response. That's why deadpool-redis includes a ping check making sure all commands have completed. If that's not the case the connection is discarded as there is currently no way to recover from this state.
2. Complete the request in the background regardless if the user of it has dropped the client. That's how tokio-postgres implemented it and it avoids the crash and burn situation.

---

Regarding this PR: Dropping the connection after every use is even worse than not using a pool at all. The returned object is definitely unusable and you're just paying the cost of having all the pool logic which never recycles any object returned to it.

I don't know if there is a way (with tokio_postgres) to implement this properly. The Client provides a cancel_token method. It does however create an additional connection to the DB which isn't great either. One solution would be to create one persistent connection to the DB which is only used for cancelling queries when connections are returned to the pool.

The cancel operation is described as "Attempts to cancel the in-progress query on the connection associated with this CancelToken." which makes me wonder what happens if there are multiple queued queries. There is currently no way to clear the message queue in tokio-postgres so I wonder if it is possible that there are multiple queued messages which would mean that a simple cancel_query call would not be sufficient.

The next steps would be:

• Check if CancelToken is a viable solution to this problem
• Get @sfackler involved to ask for a way to clear the message queue
• Create a single additional connection to the DB which is exclusively used to cancel connections

If all that is necessary I'm tempted to add a return task for taking care of such things. That would also allow for some ahead of time recycling of objects which has been a long standing issue:

• Ahead of time object creation/recycling #167

[benesch]

Regarding this PR: Dropping the connection after every use is even worse than not using a pool at all. The returned object is definitely unusable and you're just paying the cost of having all the pool logic which never recycles any object returned to it.

I'm sorry, I don't follow! What you describe is definitely not the intent of this PR. And if it works that way, it's a bug in the change as submitted.

The idea was to forcibly terminate the connection only in the case that you drop the ClientWrapper, e.g. by calling take, or if the Manager discovers that the connection is broken and cannot be recycled.

[bikeshedder]

Gotcha. Your PR description differs from what the code actually does. After being primed by your description I read the code wrong and got confused by Client and ClientWrapper. 🙈

deadpool_postgres::Client is an alias for Object (=deadpool::managed::Object<ClientWrapper>) which is responsible for returning objects to the pool. So your initial explanation sounded like you wanted connections to be closed when returning objects to the pool:

This commit makes it so that dropping a deadpool_postgres::Client forcibly terminates the PostgreSQL connection (...)

After reading your second message and reviewing the code for a second time it all makes a lot more sense now. 👍

I do wonder however if that feature of closing the connection when the client (by that I mean ClientWrapper) is dropped should better be pushed to the core of tokio-postgres. I don't want deadpool-postgres to behave very different from tokio-postgres and this could cause some serious confusion with users coming from tokio-postgres and just looking for a connection pool.

Don't get me wrong. It sounds like the proper behavior. It's just that I'm unsure if tokio-postgres would be the better place for this change.

@sfackler Sorry for pinging you twice today. What do you think of adding this feature to tokio-postgres?

[benesch]

Gotcha. Your PR description differs from what the code actually does. After being primed by your description I read the code wrong and got confused by Client and ClientWrapper. 🙈

Oh goodness, sorry about that! It was late last night and I said Client where I meant ClientWrapper.

I do wonder however if that feature of closing the connection when the client (by that I mean ClientWrapper) is dropped should better be pushed to the core of tokio-postgres. I don't want deadpool-postgres to behave very different from tokio-postgres and this could cause some serious confusion with users coming from tokio-postgres and just looking for a connection pool.

Don't get me wrong. It sounds like the proper behavior. It's just that I'm unsure if tokio-postgres would be the better place for this change.

@sfackler Sorry for pinging you twice today. What do you think of adding this feature to tokio-postgres?

I think tokio_postgres is unlikely to change, since the current API allows the user to choose whether they want graceful termination (await the connection future) or ungraceful termination (Drop the connection future).

What do you think about exposing a ManagerConfig::force_termination knob that allows users to choose which behavior they want out of deadpool_postgres?

[bikeshedder]

I think tokio_postgres is unlikely to change, since the current API allows the user to choose whether they want graceful termination (await the connection future) or ungraceful termination (Drop the connection future).

It could be a feature that improves the quality of life for developers but as you just stated it is left to the user of the library. While it would be nice if the connection was closed upon dropping the client, it is somewhat obvious by the way tokio-postgres is designed. You as the user of that library are in in full control of the connection object.

In regards to deadpool this is not the case and the connection is spawned for you. So it's fair that the pool provides a configuration option to control this behavior. 👍

I'm torn between just adding your change and adding a configuration option for it.

[umanwizard]

I'm torn between just adding your change and adding a configuration option for it.

Either would be entirely okay from our perspective! For our use case, we just need some way to force-abort connections, whether it be manual or on drop.

[bikeshedder]

It took a bit of convincing but you're totally right. deadpool-postgres should not leak workers. It should not be possible for connections to outlive the pool. This PR fixes exactly this behavior.

I really want to get deadpool 0.10 out of the door and since I consider this kind of a breaking change due to the changed behavior I would like to give this crate a new minor version, too.

Unless you're in a hurry I'd like to wait a few days for the proper deadpool 0.10 release when I need to create releases for all the deadpool-* crates anyways.

[umanwizard]

We switched to our own fork temporarily, so this issue isn't impacting us in prod. In other words, there's no rush and we're happy to wait for the release. Thanks for the help!

[benesch]

Seconding @umanwizard—thanks so much for fielding this PR and our concerns! Really appreciate all the thought you've put into deadpool's design.