TransactionMiddleware leaks transaction state
|Reported by:||Anssi Kääriäinen||Owned by:||nobody|
|Component:||Database layer (models, ORM)||Version:||1.4|
|Has patch:||no||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||yes|
TransactionMiddleware process_exception and process_response are written in a style that will leak transaction state on error. process_response for example:
if transaction.is_managed(): if transaction.is_dirty(): transaction.commit() transaction.leave_transaction_management()
now, what happens if transaction.commit() throws an error? transaction.leave_transaction_management() is never called, and the connection's transaction_state leaks an "managed" entry.
This can cause a "funny" situation where SessionMiddleware doesn't commit transactions if the SessionMiddleware is above TransactionMiddleware in the middleware stack. This is user visible in that logins do nothing - they don't give any errors, but they don't log in either.
A simple test-project attached. You need to run this under PostgreSQL so that deferred foreign keys can cause an error on commit(). And, you need to use runserver --nothreading as otherwise every request gets a new thread and a new connection with clear transaction state.
Run syncdb and then runserver --nothreading. Go to localhost:8000/view/, then go to localhost:8000/admin/ and try to log in. You can also add from django.db import connection; print connection.transaction_state to process_response to see better what happens.
I have hit this same issue under apache + mod_wsgi so this doesn't happen only in development server without threading.
The bug is present at least in versions 1.4+ of Django. I am marking this as release blocker as this is a crashing bug (logins are impossible when condition hit), and I plan to backpatch this to 1.4-master.
The fix should also be easy: add try-finally to process_response/process_exception.
Change History (23)
comment:1 Changed 4 years ago by
|Patch needs improvement:||unset|
|Triage Stage:||Unreviewed → Accepted|