Allow hooking on connection get-from-pool and release-to-pool

[Ten0]

In some cases, specific initialization/cleanup needs to be done around managed connections when fetching them from the pool and releasing them to the pool.

e.g.:

1. begin_test_transaction behavior changed in Diesel 2.0 diesel-rs/diesel#3252 with solution suggested here
2. Attempting to close any open transaction on release-to-pool (solution where transactions are generally managed with RAII as suggested here have the issues described here).

Currently this requires having custom wrappers around PooledConnection and Pool, which prevents from always using r2d2 directly (and requires reimplementing a whole bunch of traits on the wrappers).

Something that would remove this need would be to add on_take_from_pool and on_release_to_pool hooks to CustomizeConnection (or another trait if we want backwards-compat.) and/or ManageConnection.

That would solve 1. by allowing users to automatically open a test transaction on_take_from_pool and rolling it back on_release_to_pool (on_release_to_pool would have to be called before is_valid).

That would solve 2. by allowing users (and/or implementors of ManageConnection) to automatically attempt closing any open transactions on_release_to_pool in any way they see fit.

I would imagine the overhead of the CustomizeConnection one to be really-not-too-high despite the dynamic dispatch because that would be absolutely negligible compared with what we actually do with the connection. (And obviously the overhead of the ManageConnection one would be zero because the statically dispatched no-op would be optimized out.)

Would that be something that makes sense and could potentially be added to r2d2?

Thanks,

[sfackler]

2. have the issues described here)

Those issues are described in the context of an async API, but r2d2 works with blocking connections.

[Ten0]

Those issues are described in the context of an async API, but r2d2 works with blocking connections.

I think the points about "how would one handle connection errors in the Drop implementation?" (also mentionned here) are still valid in a synchronous context.
(In addition, how do you make sure to never forget to make the commit call with a such API?)

Anway, that is just to justify that it isn't easy or even necessarily desirable to "just move to an RAII transaction API" that would dodge these transaction-management-on-pooled-connection-drop issues by other means.

[sfackler]

(In addition, how do you make sure to never forget to make the commit call with a such API?)

How would you make sure to never forget a commit call in any API?

[Ten0]

How would you make sure to never forget a commit call in any API?

Well Diesel's prevents that ^^

[weiznich]

Well Diesel's prevents that ^^

I think that should be restricted to diesel::Connection::transaction, as it is easily possible to just open a transaction via diesel::connection::TransactionManager::begin without closing it later. Nevertheless I think it would be really helpful to have additional methods in CustomizeConnection that allows users to perform actions on connection checkout/checkin. Maybe even allow to control modify the is_valid()/has_broken() functions there. As far as I can tell these methods can be added without breaking change, as we could add default implementations can just call the existing functions. If you are interested in this I can open a PR next week.

[sfackler]

I am a bit confused as to how Diesel simultaneously guarantees that transactions will always be committed and needs the ability to forcibly roll back transactions when a pooled connection is returned.

In any case though, it seems reasonable to add those methods to CustomizeConnection.

[Ten0]

I am a bit confused as to how Diesel simultaneously guarantees

I realize my previous messages may have been lacking explanation to be understandable by anyone who isn't quite familiar with diesel and its issue history ^^
I'm going to try and give more details :)

---

that transactions will always be committed

It does not precisely "guarantees that transactions will always be committed", however it does provide a way for users to "make sure to never forget to make the commit call".

If you were to have a guard-based transaction API, it would probably look something like this:

let transaction = conn.begin_transaction()?;
query.execute(&mut transaction.connection);
transaction.commit()?;

Now this has several issues:

• If you rollback on dropping the guard, you don't really know what to do with errors that happen when attempting rollback.
• If you forget to make write the line with the commit call, the transaction would get rolled back (or at least attempted to be rolled back), and it's hard to check that you won't ever forget to write it.

(What I call "forget to make the commit call" is specifically "forgetting to write that last line, resulting in transaction never getting committed".)

On the other hand, Diesel's API is based on a Connection::transaction function:

conn.transaction(|conn| {
    query.execute(&mut conn);
    Ok(())
})?;

Notice several things:

• Rollback errors are easily returned by the transaction function
• It's impossible to "just forget" to specify whether to rollback or commit the transaction: that is specified the closure's Ok or Err result, and the code will obviously fail to compile if it contains neither.

Hence indeed, "always using the Connection::transaction when dealing with transactions" constitutes "a way for users to make sure to never forget to make the commit call"

Anyway - I realize now this is irrelevant to this issue, for reasons described later.

---

needs the ability to forcibly roll back transactions when a pooled connection is returned

Now there's actually two ways that this could be useful:

• people not using the transaction abstraction for some reason
• There's actually another way we can exit the transaction function, besides Ok and Err (last one I promise!): by panicking out of it.

For that second case we have several design choices:

• Catch the panic and attempt a rollback
  • blocking and possibly time consuming
  • delaying the panic unwind
  • is useless if the actual connection (not pooled) is gonna get dropped right after due to the panic)
  • possibly silently failing
• Not do anything and let the user deal with it if they want to

Notice in particular that attempting rollback only ever makes sense if the connection is pooled - otherwise it's probably fine to just let the panic unwind and drop the actual connection, which will not be reused.

Diesel went with the second approach, which in particular allows users to, depending on their use-case:

• just let the connection get dropped/closed because the program will quit soon enough that all connections will be closed hence rolled back by the database
• attempt rollback themselves, and actually log the error somewhere if it happens
• do whatever else they see fit

Now if the user's will happens to be "I know this panic in my request handling code will be catched later, and the server will continue its life normally besides that, so I want to attempt rollback and report the error to e.g. Sentry if it fails" (spoiler: that's what we do), then the current only possible approach is

having custom wrappers around PooledConnection and Pool, which prevents from always using r2d2 directly (and requires reimplementing a whole bunch of traits on the wrappers).

which having this would solve.

---

Side note: I realize now that the "no-rollback-attempt on panic" behavior and "don't forget to commit" behavior are pretty untied: it's possible to obtain both "no-rollback-attempt on panic" and "rollback-attempt on panic" in both designs, by checking whether std::thread::panicking in the guard's Drop impl, and using std::panic::catch_unwind in closure-based interface.

That makes the only relevant question between "no-rollback-attempt on panic" and "don't forget to commit" with regards to this issue the "no-rollback-attempt on panic" one:
Because we don't want a hardcoded "rollback-attempt on panic" for their previously cited downsides, it would be very handy to let users parametrize how they want to handle not-rolled-back transactions that come back to the pool.

Not to negate of course the other uses this may have, like managing a pool of connections dedicated to tests, where we would open a transaction everytime we get it from the pool, and roll it back every time it comes back, which is very close to our original issue.

[weiznich]

I've opened #131 for this.