Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No status to indicate aborted transaction. #63

Open
jenstroeger opened this issue Sep 13, 2018 · 7 comments
Open

No status to indicate aborted transaction. #63

jenstroeger opened this issue Sep 13, 2018 · 7 comments

Comments

@jenstroeger
Copy link

jenstroeger commented Sep 13, 2018

This works:

>>> tx = tm.get()
>>> tx.status
'Active'
>>> tm.commit()
>>> tx.status
'Committed'

and this does’t:

>>> tx = tm.get()
>>> tx.status
'Active'
>>> tm.abort()
>>> tx.status
'Active'

Looking at the current status code definition:

class Status:
# ACTIVE is the initial state.
ACTIVE = "Active"
COMMITTING = "Committing"
COMMITTED = "Committed"
DOOMED = "Doomed"
# commit() or commit(True) raised an exception. All further attempts
# to commit or join this transaction will raise TransactionFailedError.
COMMITFAILED = "Commit failed"

then an "Aborted" status is missing, but I think would be quite sensible and useful to have.

And having said that, it would also make sense to either export the Status publicly or add is_active() (etc.) helper functions to the ITransaction interface so that the status of a transaction can be checked conveniently.

@mgedmin
Copy link
Member

mgedmin commented Sep 14, 2018

AFAIR transaction.abort() is an alias of transaction.begin(): both abort the previous transaction and start a new one.

You're probably looking for transaction.doom(), which marks the current transaction as failed and prevents accidental commits, but doesn't start a new one.

@jenstroeger
Copy link
Author

@mgedmin, looking at the abort() code I don’t see the function being an alias, nor does it begin a new transaction. But even if that were the case, the "Active" status of a transaction after it’s been aborted should still be fixed.

@mmerickel
Copy link
Contributor

tm.get will start a new txn object if there isn’t one when configured in implicit mode. However I think this issue is about the state of the transaction object itself which is not shared across transactions handled by the manager.

@jenstroeger
Copy link
Author

I think this issue is about the state of the transaction object itself which is not shared across transactions handled by the manager.

@mmerickel, correct.

I ran into a problem where I had a transaction object tx and wasn’t able to check it’s status; I needed to know whether a corresponding data manager can still be used. The transaction manager is explicit so using get() and catching exceptions to check if tx is active seemed somewhat clumsy. And considering that tx.Status already exists, it’s easy to make that information available.

Which also revealed the second problem that abort() does not update the transaction’s status, see above.

So this issue really contains two aspects:

  1. A bug: set the transaction’s status to "Aborting"/"Aborted" upon calling abort() on it.
  2. A feature request: expose the transaction’s status properly.

@icemac
Copy link
Member

icemac commented Sep 21, 2018

This issue is maybe related to #48.

@jimfulton
Copy link
Member

Leaving #48 aside :), I'm fine with this, even though I never wanted. It's a logical thing to do to model/express the life-cycle of transactions. I'd support a (presumably simple :-]) PR.

@jenstroeger
Copy link
Author

jenstroeger commented Sep 22, 2018

@jimfulton, here you go: pull request #64 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants