Warning: Can't synchronize with repository "(default)" (Unsupported version control system "svn": No module named svn). Look in the Trac log for more information.

Ticket #1721 (closed defect: invalid)

Opened 10 years ago

Last modified 9 years ago

[PATCH] With SA-0.4, catching an exception on a database can cause exceptions in TG.

Reported by: toshio Owned by: anonymous
Priority: normal Milestone: 1.5
Component: SQLAlchemy Version: 1.0.4.3
Severity: major Keywords:
Cc: lmacken, kskuhlman

Description

If we do the following in a controller method (for instance, to catch duplicate ids)

try:
    session.save(user)
    session.flush()
except Exception, e:
    flash("Error occurred")
return dict(message="End of the line")

we create a situation where TurboGears can throw an error inside of database.py. This is bad as there is no way to catch that error inside of the application code so it propagates out to the web page.

Here's the relevant sections of database.py:

# include "args" to avoid call being pre-cached
[run_with_transaction.when("_use_sa(args)")]
def sa_rwt(func, *args, **kw):
    log.debug("New SA transaction")
    req = cherrypy.request
    req.sa_transaction = make_sa_transaction(session)

    try:
        retval = func(*args, **kw)

    except (cherrypy.HTTPRedirect, cherrypy.InternalRedirect):
        log.debug('this is only a redirect')
        # If a redirect happens; commit and proceed with redirect
        if sa_tr_active(req.sa_transaction):
            req.sa_transaction.commit()
        raise

    except:
        log.debug('this is an exception, ROLLBACK now!')
        # If any other exception happens; rollback and re-raise error
        if sa_tr_active(req.sa_transaction):
            req.sa_transaction.rollback()
        raise

    # If the call was successful; commit and proceed
    if sa_tr_active(req.sa_transaction):
        log.debug('The transaction was successful, COMMIT now!')
        req.sa_transaction.commit()

    return retval
[...]
def sa_tr_active(tr):
    if hasattr(session, 'context'):
        # SA 0.3
        return tr.session.transaction
    else:
        # SA 0.4 (effectively always active - commit or rollback don't fail)
        return True

The comment in sa_tr_active() is suspicious as the SQLAlchemy has this to say about transactions:

When using a transactional session, either a rollback() or a close() call is required when an error is raised by flush() or commit(). The flush() error condition will issue a ROLLBACK to the database automatically, but the state of the Session itself remains in an "undefined" state until the user decides whether to rollback or close.

 http://www.sqlalchemy.org/docs/04/session.html#unitofwork_managing

I'm not sure if this should be fixed by changing sa_tr_active() to return False when the transaction has encountered an error that should be rolled back or if sa_rwt() should catch errors when it does a commit() and do a rollback() instead but one of those two places needs to change.

Attachments

turbogears-sqlalchemy-0.4-rollback.patch Download (1.5 KB) - added by toshio 10 years ago.
Implement exception checking and rollback in sa_rwt()
session.close.patch Download (2.4 KB) - added by paj 10 years ago.
turbogears-sqlalchemy-0.4-rollback-test.patch Download (1.2 KB) - added by toshio 10 years ago.
Test case for this issue

Change History

Changed 10 years ago by toshio

Implement exception checking and rollback in sa_rwt()

comment:1 Changed 10 years ago by toshio

Patch to implement the second of the two choices added. This has been tested working with SQLAlchemy 0.4.2p3 and TurboGears-1.0.4.2.

A test project can be found here:  http://toshio.fedorapeople.org/snifflemap.tar.gz

For easy testing you can change the dburi from mysql to sqlite.

The process_registration() method will show the error. Hit this URL twice in a row. The first time will add the user to the db. The second time will raise an IntegrityError? due to the unique constraint on primary keys being violated and lead to the problem in sa_rwt():;

 http://localhost:8080/process_registration?user_id=terrance&password=tom

comment:2 Changed 10 years ago by toshio

  • Summary changed from With SA-0.4, catching an exception on a database can cause exceptions in TG. to [PATCH] With SA-0.4, catching an exception on a database can cause exceptions in TG.

comment:3 Changed 10 years ago by paj

  • Component changed from TurboGears to SQLAlchemy

comment:4 Changed 10 years ago by lmacken

  • Cc lmacken added

comment:5 Changed 10 years ago by paj

Hi, I'm the author of that cryptic comment. What I was getting at is that if no transaction is in progress, commit and rollback just quietly do nothing. This was different in SA 0.3 where they would raise an exception in that case.

I'm not sure about the patch as it stands. It will silently swallow any exceptions during commit - which could result in unpredictable behaviour of an app. One option is to use a try/finally block around everything, with session.close() in the final bit.

I don't think there's any reasonable way of sa_tr_active knowing whether commit will cause an error. This will usually be session.flush() failing, perhaps because you violated a database constraint.

Changed 10 years ago by paj

Changed 10 years ago by toshio

Test case for this issue

comment:6 Changed 10 years ago by toshio

I've attached a test case for this issue. Unfortunately, session.close.patch doesn't fix it.

The following behaviors are what needs to occur:

  1. The app's controller creates a situation where the db will reject the transaction
  2. The app does not attempt to commit the transaction
  3. The exception raised in sa_rwt() needs to be displayed.
  4. The session must be closed.

This is the situtation handled by:

    # If the call was successful; commit and proceed
    if sa_tr_active(req.sa_transaction):
        log.debug('The transaction was successful, COMMIT now!')
        req.sa_transaction.commit()

plus the session.close.patch.

  1. the app provokes the exception but does not handle it
  2. sa_rwt() needs to catch the exception and display it
  3. sa_rwt() needs to close the session

This is provided by this section of code:

    except:
        log.debug('this is an exception, ROLLBACK now!')
        # If any other exception happens; rollback and re-raise error
        if sa_tr_active(req.sa_transaction):
            req.sa_transaction.rollback()
        raise

plus the session.close.patch

  1. the app provokes the exception and handles it itself.
  2. sa_rwt() needs to know not to try to commit the transaction as the app has already handled the error.
  3. sa_rwt() needs to close the transaction.

In SQLAlchemy-0.3 this is handled by:

    # If the call was successful; commit and proceed
    if sa_tr_active(req.sa_transaction):
        log.debug('The transaction was successful, COMMIT now!')
        req.sa_transaction.commit()

plus the session.close.patch. In SQLAlchemy-0.4 this situation is unhandled due to sa_tr_active() always returning True.

So it looks to me as though the fix needs to be a combination of session.close.patch and a smarter version of sa_tr_active().

comment:7 Changed 10 years ago by toshio

The controller method can provide a hint here by attempting to rollback() or close() the session before returning. Testing that, however, didn't seem to have any effect on whether sa_rwt() issued an exception or not so if that's needed, that's not all that's needed.

comment:8 Changed 10 years ago by chrisz

I'm just working on this. Just to clarify beforehand: TG 1.0 sessions are *not transactional*, so the passage you quoted at the beginning does not apply.

comment:9 Changed 10 years ago by chrisz

I have checked in the session close patch now (closing the session in SA 0.3 does not harm, so I removed the case distinction). This should at least solve the problems with SA 0.4.3. I'll look at the original ticket issue later.

comment:10 Changed 10 years ago by chrisz

Ok, I think I understand the original problem now. So far I was only trying to get the current unit tests running with SA 0.4.3. They are running now with the session close patch, but only because toshio's test case is not yet covered by the current tests.

I think toshio's analysis is correnct. paj's statement that "if no transaction is in progress, commit and rollback just quietly do nothing" is unfortunately not true any more in SA 0.4.3 and that's causing the trouble. If no transaction is in progress, we are now getting this InvalidRequestError "The transaction is inactive due to a rollback in a subtransaction and should be closed".

I agree that one of the two originally proposed solutions is necessary. More exactly, they could be implemented as follows:

Solution 1: Change sa_tr_active() to return session().transaction.is_active instead of True.

Solution 2: Catch exceptions in the commit() and rollback() calls, but only of type sqlalchemy.InvalidRequestError - this will diminish paj's objection that we would catch real exceptions in commit that should be raised up to the app.

Solution 1 seems better, but I'm not sure whether the transaction and is_active attributes are considered part of the SA API, because I don't find them in the docs. I'll ask on the SA mailing list.

comment:11 Changed 10 years ago by chrisz

As suggested by jek, another variant of solution 1 is making use of the fact that session.begin() returns the transaction, instead of evaluation it with session().transaction. It then needs to be stored in the request, because it may be changed on restart_transaction. In fact this is already done, in the attribute sa_transaction for SA 0.3. But unfortunately, for SA 0.4, the session is stored here instead of the transaction, and we cannot change this back so easily for compatibility reasons. Need to think about this a little bit more.

comment:12 Changed 10 years ago by chrisz

  • Status changed from new to closed
  • Resolution set to fixed

I have now checked in a complete fix with test in r4132. The transaction is now stored in the request so we can determine if is still active before committing anything. I think we can accept the small incompatibility that request.sa_transaction is now always the SA transaction, and not the SA session in the case of SA >= 0.4. This is even more in line with what is documented at  http://docs.turbogears.org/1.0/SQLAlchemy, and it is also less confusing. I will note this in the changelog anyway. Please reopen this ticket if you think there is any problem with this fix.

comment:13 Changed 9 years ago by toshio

Thanks! Verified working here. This patch has been applied to the Fedora Packages for Fedora 8 and will be available to users there in a short time.

comment:14 Changed 9 years ago by kvdb

  • Priority changed from high to normal
  • Status changed from closed to reopened
  • Resolution fixed deleted

Looks like this issue has resurfaced in the TG 1.5 branch using elixir 0.6.0 and SA-0.5beta3.

comment:15 Changed 9 years ago by kskuhlman

  • Cc kskuhlman added

All tests pass for me with tg1.5, elixir 0.6.0 & sa 0.5.0beta3. Can you provide a test case that shows the problem you're having?

comment:16 Changed 9 years ago by kvdb

  • Status changed from reopened to closed
  • Resolution set to invalid

False alarm. Sorry.

Seems the commit() was actually performed when the exposed function got out of scope (ended). The try/except inside it obviously wasn't catching any errors.

Note: See TracTickets for help on using tickets.