search

oxidecomputer / async-bb8-diesel

Safe asynchronous access to Diesel and the bb8 connection manager
MIT License
12 stars 8 forks source link

`transaction_async` is not cancel-safe or panic-safe, and bedlam ensues #47

Open davepacheco opened 1 year ago

davepacheco commented 1 year ago

I think transaction_async was ported to async from similar synchronous code. But it looks to me like it's not cancel-safe because if it is cancelled at this await point then we will wind up having executed BEGIN TRANSACTION; and then putting the connection back in the pool. The result of this is surprisingly terrible: any future operation (using transaction_async() or not) may wind up getting this connection, and it may wind up successfully executing any number of SQL statements (including UPDATEs and SELECTs) that appear to work normally, but none of the changes are reflected in the database because they're being made in a not-yet-committed transaction. This can go on indefinitely -- any number of statements over any amount of time. The connection is essentially "poisoned" in a way that updates are going to /dev/null except that reads (on this one connection only) do reflect those changes.

I have reproduced this with a (relatively) simple demo.

davepacheco commented 1 year ago

Note that this is in practice much less likely to be a problem for Omicron after oxidecomputer/dropshot#701.

davepacheco commented 3 days ago

It appears that @jmpesp saw a similar failure mode where the function passed to transaction_async panicked (rather than being cancelled). The result is that the connection was returned to qorb still inside a transaction. This is slightly different than being cancelled, but the effect is the same (a connection escapes without being reset to its nominal state) and a possible fix could be the same (having a Drop handler deal with this).