Permit recovery of failed transaction commit or rollback

[dancingbug]

At present, Transaction::commit and Transaction::rollback have signatures
pub fn commit (mut self) -> Result<(), FbError> and
pub fn rollback(mut self) -> Result<(), FbError>

Can these ever fail without actually releasing resources? I found this and this which indicate that it's been a problem in the past.

If this does happen, the caller can't even attempt to recover straightforwardly because these methods consume the transaction.

[fernandobatels]

I think that is a conception case. If user want's the recovery possibility, they need use the _retaining variations. The rusqlite use the same approach(transaction consume) for commit/rollback.

What is your opinion @jairinhohw ?

[jairinhohw]

I think the only reason a commit or rollback would fail is a connection error (as stated in the issues linked), so this might have been a problem in the < 3.0 versions of the firebird, but for 3.0 and above, the connection is encrypted, so a network error makes the whole connection invalid, as the encryption state gets de-synchronized, so i don't this this is a problem in most of the cases, as recovery of the transaction is impossible.

[dancingbug]

I do think for it's appropriate to consume the transaction in both cases, since otherwise the user has to always think about whether drop() will be called.

But I also do think there should be a 'standard' way (ie in the high-level crate) to say "I want to do a commit/rollback without retaining, but I want to hold on to the transaction or get it back if there's a problem", with documentation warnings about trying to reuse closed handles.

Maybe a more interesting reason why this could matter is that, semantically there is (or, there can be) a difference: choosing whether to borrow or consume gives more fine grained possibilities to an implementer of FirebirdClient to decide what to do based on the user's actual intention.
For example a FirebirdClient might only see commit where a user means "I'm done with the transaction after this." but if the FirebirdClient implements extra logic that happens before actually committing (it could be a mock, do validation on the network connection, execute in a lazy way, be a shim for another client or even database) then it would be nice to have a simple, ergonomic way for the user to provide a recovery path if it fails before the resource handle is invalidated.

Regarding the previous comment about versions: actually I work with an old Firebird version (< 2.5 is all I will say here) from a very old corporate Delphi codebase. Some of my interest in this crate comes from the ease of using Rust's C ffi to talk with Delphi with little to no overhead.

For those reasons I still propose to add some way to either keep the transaction, or else get back the transaction with an error.

The simplest I could think of would be separate borrowing methods, one for commit and one for rollback.
What do you think of that?

[fernandobatels]

Ok, its a nice point.

Maybe we can make two versions of transaction implementation commit/rollback methods using cargo features. The default still the consume way.

[jairinhohw]

I propose to create a new function like this (one for commit, and other for rollback):

/// Rollback the current transaction changes, returning the transaction on error
pub fn rollback_recoverable(mut self) -> Result<(), (FbError, Self)> {
    if let Err(e) = self.data.rollback(self.conn) {
        return Err((e, self));
    }

    ManuallyDrop::new(self);

    Ok(())
}

This will allow to recover the transaction on failure, and avoid allowing the user to call commit / rollback more than once

[fernandobatels]

I propose to create a new function like this (one for commit, and other for rollback):

/// Rollback the current transaction changes, returning the transaction on error
pub fn rollback_recoverable(mut self) -> Result<(), (FbError, Self)> {
    if let Err(e) = self.data.rollback(self.conn) {
        return Err((e, self));
    }

    ManuallyDrop::new(self);

    Ok(())
}

This will allow to recover the transaction on failure, and avoid allowing the user to call commit / rollback more than once

Good idea. This will be part of default feature or only available with a special cargo feature?

[jairinhohw]

I propose to create a new function like this (one for commit, and other for rollback):

/// Rollback the current transaction changes, returning the transaction on error
pub fn rollback_recoverable(mut self) -> Result<(), (FbError, Self)> {
    if let Err(e) = self.data.rollback(self.conn) {
        return Err((e, self));
    }

    ManuallyDrop::new(self);

    Ok(())
}

This will allow to recover the transaction on failure, and avoid allowing the user to call commit / rollback more than once

Good idea. This will be part of default feature or only available with a special cargo feature?

I don't see a reason to add a cargo feature to enable this, as it will not replace the existing commit and rollback.

[dancingbug]

I'd like to leave this issue open for a little bit to explore other ideas. If you want to make a PR and just get something working now I think that would be okay.
If you find interesting approaches from other libraries it would be nice to mention them here.

Some further thoughts on this:
It's too bad Rust doesn't have proper linear types ('must-use exactly once' types). I really see clearly now what people mean when they say rust only has affine types ('can use at most once' types) .

If we had something like that we could have a very elegant way like for example Transaction<NotCommitted> and Transaction<Committed> where the parameters are just marker types, and the compiler complains if the wrong marker type is being used and it goes out of scope (so you are forced to pass Transaction<NotCommited> to commit or rollback to get back a Transaction<Commited> which is allowed to go out of scope)

I wonder if there's a way to emulate this with not too much overhead. Maybe similar to what rusqlite does with set_drop_behavior

another complicated solution, but one which provides a whole different way of looking at sequential operations, is to only allow access to transactions after failure using closures, either in the function arguments (FirebirdClient chooses when to execute) or on a wrapper in the return type (user gets to choose). This would be something like std::result::Result::map_or_else (or the similar map_err, or and_then if we want to chain several things)
This is more of a functional programming approach.

[dancingbug]

@fernandobatels @jairinhohw I think i found the solution I was looking for to this and to #61
To me it's very exciting this is possible in Rust!! Here is a proof of concept: (edit- updated link)
link to play.rust-lang.org gist
If you are not used to this kind of thing, it will seem a little strange, but trust me it is worth it to understand
If you want me to annotate all the types and add more comments, please let me know.

Please try to see if you can figure out a way to call with_transaction in main() where you run an execute on the transaction at least once but then do not call commit, and do not end all error handling chains with rollback.
You should do so without "cheating" (Ie don't force a Result<A,B> into an A or a B using something like unwrap(), and don't use the const PhantomData<S> values defined at the top in your attempts because we would hide them from the user in a real implementation)

advantages:

• Can't immediately call commit or rollback on a Transaction as soon as your closure receives it. You actually have to do something with it first like execute a statement.
• Impossible to (with safe rust) call commit twice in a row if the first one was successful (and similarly for other combinations of rollback and commit)
• All methods of Transaction are &mut self allowing the user to decide what to do in case of error, but they can't drop it.
• The user is forced to handle all errors before returning, in order to obtain a Token<Consistent>. (we can provide helpers for this to avoid all the match arms)
• When the closure provided to with_transaction returns, we are statically guaranteed the last method run on it was either a successful commit or a rollback, so it's safe to drop without doing anything.
• Presumably small or zero extra runtime cost (compared to just Result<(), FbError>) because PhantomData is a zero sized type (except maybe to do with one function pointer passed to with_transaction).
• All guaranteed at compile time so long as the user is using safe rust.

You can think of this like a compile-time verified Finite State Machine using the type system. This is possible in Rust only because it has a concept of 'moves' which here are used to statically disallow reuse of the Token type.

edit: btw this kind of approach is called the TypeState Pattern (edit:different article)

[jairinhohw]

While i have some concerns about the error handling part, we can work on it when trying to implement, however there is a bigger problem.

The existing Drop implementation in the Transaction exists for the user to be able to create a transaction with Transaction::new, however with this approach we cannot guarantee that the user will commit or rollback the transaction outside of the with_transaction closure.

Also the existing with_transaction, while does not force the user to handle directly all errors, does rollback or commit the transaction based on the output of the closure (if it is an Err, we rollback, else we commit). This works well in the majority of cases, and when it does not, my idea was to use the Transaction directly (so adding the rollback_retaining and commit_retainig variants).

[dancingbug]

Yes, this would require to make Transaction::new not part of the API so the user can only ever obtain an &mut Transaction inside their closure passed to with_transaction (constructed/reused and released by Connection only) or something equivalent (maybe it is still possible to hand out a &mut Transaction managed by Connection without using a closure and still have the compile-time guarantees. but I have a feeling it's essentially the same)

Anyways I won't be pushy if it's too far from rusqlite or otherwise is not wanted, but one way or another I'm going to get this implemented (in a separate crate because the changes will be big) 😁 and I hope the benefits will become apparent.

[jairinhohw]

I like the idea of having only the connection create and manage the transactions, as i wanted to have just one transaction per connection that is reused by default.

I don't know for sure, but i think it would be better to add the marker to the Transaction itself, to not have to manage both the transaction and the token, but it will need to change the with_transaction to pass an owned transaction and then return it to the connection.
I can't think of a way to allow using the ? operator this way though.

[fernandobatels]

@fernandobatels @jairinhohw I think i found the solution I was looking for to this and to #61
To me it's very exciting this is possible in Rust!! Here is a proof of concept: (edit- updated link)
link to play.rust-lang.org gist
If you are not used to this kind of thing, it will seem a little strange, but trust me it is worth it to understand
If you want me to annotate all the types and add more comments, please let me know.

Please try to see if you can figure out a way to call with_transaction in main() where you run an execute on the transaction at least once but then do not call commit, and do not end all error handling chains with rollback.
You should do so without "cheating" (Ie don't force a Result<A,B> into an A or a B using something like unwrap(), and don't use the const PhantomData<S> values defined at the top in your attempts because we would hide them from the user in a real implementation)

advantages:

* Can't immediately call `commit` or `rollback` on a `Transaction` as soon as your closure receives it.  You actually have to do something with it first like execute a statement.

* Impossible to (with safe rust) call `commit` twice in a row if the first one was successful (and similarly for other combinations of `rollback` and `commit`)

* All methods of `Transaction` are `&mut self` allowing the user to decide what to do in case of error, but they can't drop it.

* The user is forced to handle all errors before returning, in order to obtain a `Token<Consistent>`. (we can provide helpers for this to avoid all the match arms)

* When the closure provided to `with_transaction` returns, we are statically guaranteed the last method run on it was either a successful `commit` or a `rollback`, so it's safe to drop without doing anything.

* Presumably small or zero extra runtime cost (compared to just `Result<(), FbError>`) because `PhantomData` is a zero sized type (except maybe to do with one function pointer passed to `with_transaction`).

* All guaranteed at compile time so long as the user is using safe rust.

You can think of this like a compile-time verified Finite State Machine using the type system. This is possible in Rust only because it has a concept of 'moves' which here are used to statically disallow reuse of the Token type.

edit: btw this kind of approach is called the TypeState Pattern (edit:different article)

Very nice article

[fernandobatels]

So, I think we can keep idea of implement the '_retaining' methods for now. This is a more simple approach.