KeyboardInterrupt and SystemExit should not be wrapped by sqlalchemy

[Former user]

The SQLAlchemy database engine exception handling currently wraps all exceptions during an operation in the database engine, and presents an sqlalchemy.exceptions.SQLError instance instead. This is good in most cases.

Two exceptions should not be wrapped this way: KeyboardInterrup and SystemExit. Those exceptions are generated, respectively, when the user requests the program should be interrupted (e.g. via Ctrl+C during a Python interactive prompt), and when the program has been asked to quit.

Instead, SQLAlchemy should allow these two exception types to propagate up without being caught; certainly they should not propagate as an SQLError.

An example session:

>>> import sqlalchemy as sqla
>>> db = sqla.create_engine('sqlite:///test.db')
>>> metadata = sqla.BoundMetaData(db)
>>> test = sqla.Table('test', metadata, sqla.Column('id', sqla.Integer, primary_key=True), sqla.Column('name', sqla.String(40)))
>>> test.create()

>>> test_insert = test.insert()
>>> for dummy in range(10000):
...     test_insert.execute(name="Fred Test")
...
<sqlalchemy.engine.base.ResultProxy object at 0x303be450>
<sqlalchemy.engine.base.ResultProxy object at 0x303be630>
Traceback (most recent call last):
  File "<stdin>", line 2, in ?
  File "/usr/lib/python2.4/site-packages/sqlalchemy/sql.py", line 1175, in execute
    return self.compile(engine=self.engine, parameters=compile_params).execute(*multiparams, **params)
  File "/usr/lib/python2.4/site-packages/sqlalchemy/sql.py", line 1064, in execute
    return e.execute_compiled(self, *multiparams, **params)
  File "/usr/lib/python2.4/site-packages/sqlalchemy/engine/base.py", line 783, in execute_compiled
    return connection.execute_compiled(compiled, *multiparams, **params)
  File "/usr/lib/python2.4/site-packages/sqlalchemy/engine/base.py", line 571, in execute_compiled
    self._execute_raw(context)
  File "/usr/lib/python2.4/site-packages/sqlalchemy/engine/base.py", line 585, in _execute_raw
    self._autocommit(context.statement)
  File "/usr/lib/python2.4/site-packages/sqlalchemy/engine/base.py", line 496, in _autocommit
    self._commit_impl()
  File "/usr/lib/python2.4/site-packages/sqlalchemy/engine/base.py", line 488, in _commit_impl
    raise exceptions.SQLError(None, None, e)
sqlalchemy.exceptions.SQLError: (KeyboardInterrupt)  None None

The user pressed Ctrl+C during the insert of a record; this should propagate the KeyboardInterrupt directly out, instead of an SQLError. Similarly for a SystemExit exception.

[Former user]

Replying to guest:

Two exceptions should not be wrapped this way: KeyboardInterrup and SystemExit. Those exceptions are generated, respectively, when the user requests the program should be interrupted (e.g. via Ctrl+C during a Python interactive prompt), and when the program has been asked to quit.

Instead, SQLAlchemy should allow these two exception types to propagate up without being caught; certainly they should not propagate as an SQLError.

More succinctly: The two exceptions KeyboardInterrupt and SystemExit are not errors, so wrapping them as SQLError is wrong. They should be allowed to propagate directly.

Here is how Frederik Lundh describes the problem and resolution: http://effbot.org/zone/stupid-exceptions-keyboardinterrupt.htm

[Former user]

Patch against SQLAlchemy trunk as of 20070727 to fix this ticket

[Former user]

Patch against SQLAlchemy-0.3.8 to fix this ticket

[Former user]

As described in the "What's New in Python 2.5" document at "PEP 352: Exceptions as New-Style Classes", Python 2.5 places these two non-error exceptions outside the 'Exception' inheritance hierarchy for exactly this purpose.

Until SQLAlchemy drops support for versions of Python earlier than 2.5, the fix described in this ticket is the correct approach.

[Mike Bayer]

im a little confused why theres two patches, one against release 0.3.8 which is two months out of date. preferable would be against 0.4 which is to be merged to trunk very soon (and also has a lot more of these catches).

also i dont agree the patch is the completely the "correct" approach; not every exception is meant to be re-raised here, particularly those within the connection pool where errors on close are logged and consumed, and the connection is recycled (not unlike how exceptions that occur within del are automatically consumed by Python). if KeyboardInterrupt/SystemExit should be reraised then it should be limited to those two.

also theres an assumption here that every KeyboardInterrupt and SystemExit is going to end the application; such as in the main _execute() method where all the connection cleanup code is skipped; if enclosing code suppresses either exception, the connection is not closed, and a subsequent table operation by another connection can then deadlock the application (and especially with an exception condition, as the open connection is still referenced within the stack frames referenced by the traceback).

easier here would be along these lines:

catch Exception, e:
     <cleanup code>
     if isinstance(e, (KeyboardInterrupt, SystemExit)):
         raise e
     else:
         raise exceptions.SQLError(....)

thats if youre willing to have a re-targeted stacktrace, which i dont think should be an issue if you just want the exception itself propagated.

[Former user]

im a little confused why theres two patches, one against release 0.3.8 which is two months out of date. preferable would be against 0.4 which is to be merged to trunk very soon (and also has a lot more of these catches).

I made the patch against 0.3.8 because that's what I'm running, and I wanted a patch to accompany this ticket so it was clearer. I subsequently patched trunk as well, ignorant of the 0.4.0 branch. Feel free to use whichever one makes more sense.

[Former user]

also i dont agree the patch is the completely the "correct" approach

I'm not very familiar with the code base. So long as the approach in http://effbot.org/zone/stupid-exceptions-keyboardinterrupt.htm is followed, this ticket is satisfied.

also theres an assumption here that every KeyboardInterrupt? and SystemExit? is going to end the application

If they are being consumed elsewhere, that's another bug (or another instance of this one). Yes, probably cleaning up before re-raising is a good idea just in case.

[Mike Bayer]

any issue with using isinstance(exception) and issuing a re-targeted stacktrace ? effbot's approach is a little inconvenient here.

[jek]

2d8b5bb4f36e5624f25b170391fe42d3bfbeb623

[Former user]

Where has this been fixed? The 0.4.0 trunk still seems to do exactly what this ticket describes as the bug: catching with "except Exception", instead of allowing the two non-error exceptions to raise intact.

[Former user]

any issue with using isinstance(exception) and issuing a re-targeted stacktrace ?

The original exception is the one that needs to be raised (in the case of KeyboardInterrupt and SystemExit).

effbot's approach is a little inconvenient here.

Yes, you're right. That's why this is fixed in Python 2.5. Unfortunately, this leaves the choice between abandoning Python versions before 2.5, or using the inconvenient special-casing of non-error exceptions.

[jek]

Replying to guest:

Where has this been fixed?

from the changelog "DBAPIError and subtype constructors will refuse to wrap a SystemExit? or KeyboardInterrupt?, returningthe original interrupt exception instead of a new instance. "

[Former user]

Replying to jek:

Replying to guest:

Where has this been fixed?

from the changelog "DBAPIError and subtype constructors will refuse to wrap a SystemExit? or KeyboardInterrupt?, returningthe original interrupt exception instead of a new instance. "

Thanks, the original close message didn't make this clear.

[Mike Bayer]

Removing milestone: 0.4.0 (automated comment)