Drop impl for Transaction has silent side effects

[dancingbug]

In number #58 we eliminated all but one path toward executing the Drop::drop impl of Transaction

While working on that PR i was thinking about whether there really should be side effects in a Drop impl at all (for example rolling back)

Was a conscious decision made when Drop was implemented at first?

I actually don't know what the idiomatic way is in Rust here, but it seems like it could be a silent source of bugs or unintended behavior, for example if the transaction handle was already invalidated by some other means (maybe even not implemented yet- every new mut self method will have to be on guard for such behavior)

Alternative ideas-

• Panic to loudly let the user know they forgot to deal with it?
• Let it die and be the user's problem?
• set_drop_behavior like rusqlite, but where the user is forced to make an initial choice in Transaction::new() ? that way they can't manually construct a Transaction without being made aware of the behavior.
• Maybe TransactionData should actually be managed by Connection and not Transaction? I wonder if there could be a trick here using lifetimes (say if TransactionData is borrowed by Transaction I think it would be the other way around. If TransactionData<'a> held a &mut 'a Transaction then it would mean we had to drop TransactionData before the Transaction)
• Maybe Transaction as it is, isn't the right abstraction to leverage Rust safety. Maybe it should really be TransactionBuilder<InconsistentState> (needs commit/rollback) and TransactionBuilder<ConsistentState> (no pending writes) and you can only pass the ConsistentState one to a Connection to execute. These could potentially be implemented as Futures` (but we just run them in a single thread because of Firebird limitations). I'm not sure if that would mean having to use a bunch of complicated async/await machinery or not.

relates to #56

Thoughts?

[jairinhohw]

The transaction calls rollback in the drop code to avoid not freeing the resources associated with it (works kind of a RAII, as creating a transaction will create it in the firebird server, and to destroy the transaction you need to call rollback or commit, else the transaction will be valid for as long as the connection is open in the server, with no way to actually use it if the handle in the client was lost).

To panic in the drop code is a valid alternative, but i don't like the idea to do this, as it would complicate quite a lot the error handling (basically impossible to use the ?, as you wold always need to match the result and rollback on failure).

[jairinhohw]

* Let it die and be the user's problem?

I don't like this idea, as it is really easy to not destroy the transaction this way

* `set_drop_behavior`  like rusqlite, but where the user is forced to make an initial choice in `Transaction::new()` ? that way they can't manually construct a Transaction without being made aware of the behavior.

This one seems ok to me

* Maybe `TransactionData` should actually be managed by `Connection` and not `Transaction`? I wonder if there could be a trick here using lifetimes (~say if `TransactionData` is borrowed by `Transaction`~ I think it would be the other way around. If `TransactionData<'a>` held a `&mut 'a Transaction` then it would mean we had to drop `TransactionData` _before_ the `Transaction`)

I though of creating a "default transaction" that is managed by the connection and is reused for the lifetime of it, but did not find a good way to implement this yet

* Maybe `Transaction` as it is, isn't the right abstraction to leverage Rust safety. Maybe it should really be `TransactionBuilder<InconsistentState>` (needs commit/rollback) and `TransactionBuilder<ConsistentState>` (no pending writes) and you can only pass the `ConsistentState` one to a `Connection` to execute. These could potentially be implemented as `Future`s` (but we just run them in a single thread because of Firebird limitations). I'm not sure if that would mean having to use a bunch of complicated async/await machinery or not.

I did not really understand this one, why would TransactionBuilder<InconsistentState> not be able to execute statements? It is not that rare to use a transaction to make more than one inserts / updates and have them all rolled back in case any of them fails