Diesel: Transaction does not close properly when failing on commit

[Ten0]

Setup

Versions

• Rust: latest stable
• Diesel: master
• Database: postgres
• Operating System linux

Feature Flags

• diesel: postgres

Problem Description

Connection gets released to the connection pool with transaction not being closed.
Then triggering "AlreadyInTransaction: Cannot perform this operation while a transaction is open" errors.

What are you trying to accomplish?

Serializable transaction fails on transaction commit:
https://github.com/diesel-rs/diesel/blob/5369a7a3d13b1323fcac9acf9d13256f19a970a0/diesel/src/pg/transaction.rs#L276
"DatabaseError: could not serialize access due to read/write dependencies among transactions"
Connection is released to the pool (r2d2) and should be usable again.

What is the expected output?

Next requests successfully start their own transaction.

What is the actual output?

AlreadyInTransaction: Cannot perform this operation while a transaction is open

Related issues

https://github.com/diesel-rs/diesel/issues/2123
In my case, transactions are not nested but I suspect this is the same issue.

Solving

I suspect catching the DatabaseError on commit_transaction and calling rollback_transaction would solve this issue.

Checklist

• [X] I have already looked over the issue tracker for similar issues.
• [X] This issue can be reproduced on Rust's stable channel. (Your issue will be
  closed if this is not the case)

[weiznich]

I think explicitly handling different error cases there should be the right solution. I would propose the following way forward:

Check the result of commit_transaction explicitly there, if it returns an Err(AlreadyInTransaction) try running abort_transaction the current running transaction. Now this can fail or succeed. In the failure case I would return the error of abort_transaction, in the success case the error from the original commit_transaction call to communicate that the transaction failed. Additionally we should document this behaviour in the API docs.
Beside of that I would assume that as similar thing can happen here, so we should make the same change there?

As I'm currently do not have endless time to work on diesel, it would be great to get a PR for this.

[Ten0]

Perfect! I will try to open a PR for this.

Just to make sure we're aligned though:

Check the result of commit_transaction explicitly there, if it returns an Err(AlreadyInTransaction) try running abort_transaction the current running transaction.

The error we would catch here is instead any DatabaseError: the error that triggers when the transaction fails would usually be the serialization failure, which is not an AlreadyInTransaction error.
The AlreadyInTransaction error triggers when trying to open a new transaction when that connection with a non-closed transaction is released to the pool and reused.

[weiznich]

Oh, yes that makes sense. Matching on Err(DatabaseError(SerializationFailure)) is the right way.

[Ten0]

Matching on Err(DatabaseError(SerializationFailure)) is the right way.

I don't think there is a SerializationFailure subtype for DatabaseError, is there ? Also, I'm not sure if a SerializationFailure there is the only DatabaseError reason for which the transaction would fail to commit, but if not, I would beleive the desirable behaviour would still be to rollback the transaction before releasing it to the pool.

Given all that, wouldn't it be better to just match on any DatabaseError there ?

[weiznich]

Ok, so from looking at the variants of DatabaseError again: ReadOnlyTransaction and SerializationFailure are cases that can explicitly happen while committing a transaction. I do not think that the other variants are relevant here, as the first two are catched by the transaction itself, while UnableToSendCommand will likely fail any query. Beside of that I think it could be reasonable to have a wild card match for the other cases there, if someone can come up with an case that's not ReadOnlyTransaction or SerializationFailure. (I just want to be sure that we don't try things on a connection that is broken by some other cause, like random network problems)

[Ten0]

Beside of that I think it could be reasonable to have a wild card match for the other cases there, if someone can come up with an case that's not ReadOnlyTransaction or SerializationFailure.

I agree :)
DatabaseError(__Unknown, "current transaction is aborted, commands ignored until end of transaction block") after a recover of an error for instance.

I just want to be sure that we don't try things on a connection that is broken by some other cause, like random network problems

That is surely a desirable behaviour, though I imagined they were all outside of DatabaseError. But yes UnableToSendCommand can be excluded as well.

So do we agree we want to:

match transaction_manager.commit_transaction(self.connection) {
    Err(DatabaseError(UnableToSendCommand, _)) => {}
    Err(DatabaseError(_, _)) => { /*try rollback*/ }
    _ => {}
}

I do not think that the other variants are relevant here, as the first two are catched by the transaction itself

If I understand correctly you mean that UniqueViolation or ForeignKeyViolation could only trigger in previous queries, not on commit. If that behaviour were to change though (like, for some database system it would trigger on commit if another transaction that removes a line with a FKey we linked to was committed between when we added our own line and when we committed), wouldn't the desirable behaviour still be to rollback before releasing the connection ? I would think so, hence no need to exclude them from the rollback.

[weiznich]

I agree :)
DatabaseError(__Unknown, "current transaction is aborted, commands ignored until end of transaction block") after a recover of an error for instance.

I would prefer returning the original error, returned from the database stating why it's not possible to complete the transaction.

If I understand correctly you mean that UniqueViolation or ForeignKeyViolation could only trigger in previous queries, not on commit. If that behaviour were to change though (like, for some database system it would trigger on commit if another transaction that removes a line with a FKey we linked to was committed between when we added our own line and when we committed), wouldn't the desirable behaviour still be to rollback before releasing the connection ? I would think so, hence no need to exclude them from the rollback.

I think DatabaseError is more or less a direct mapping from the SQLSTATE codes returned by the database. After looking at the corresponding wikipedia page it seems like only codes starting with 25xxx are relevant here. Therefore I would prefer keeping the match there as small as possible to not cover other error cases, espessially not UniqueViolation or ForeignKeyViolation, because if they turn up the database is either not following the standard (which likely causes problems in other shared parts of diesel and should have a separate impl somewhere else) or something is completely broken (which we want to know). If it turns out later that we've missed some important case we can add it later anyway.

[Ten0]

Alright, does this implementation look good to you ? :)